Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR: Add utility function to get raw bytes from QImages (compat.py) #355

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zjp
Copy link
Contributor

@zjp zjp commented Jul 16, 2022

We use this utility function in ChimeraX to grab the raw content of a QImage to pass to our renderer. I'm hoping it will be useful to other users as well, and I'm open to feedback about how to make that possible.

@dalthviz
Copy link
Member

Thanks @zjp ! Then this is comming from ChimeraX source code, right? Is there any licensing we should know about?

If that is the case I would say that the license should be compatible with MIT (the license we use here) and we should add a note in the function pointing out from where the function is comming from.

Besides that adding a tests for this could be nice too :)

Maybe @CAM-Gerlach and/or @ccordoba12 have more elements to have in mind.

@zjp
Copy link
Contributor Author

zjp commented Jul 18, 2022

There's an equivalent function but it needed adaptation to your project. There's no licensing concerns on your end -- my goal is to upstream this code and remove it from ChimeraX.

I'll add tests!

@dalthviz dalthviz added this to the v2.2.0 milestone Jul 18, 2022
@dalthviz dalthviz changed the title compat.py: Add utility function to get raw bytes from QImages PR: Add utility function to get raw bytes from QImages (compat.py) Jul 19, 2022
@ccordoba12
Copy link
Member

@zjp, this has a conflict with master now.

@zjp
Copy link
Contributor Author

zjp commented Jul 22, 2022

When I get a chance I'll rebase thanks for letting me know!

qtpy/compat.py Outdated Show resolved Hide resolved
@CAM-Gerlach
Copy link
Member

Thanks @zjp ! Then this is comming from ChimeraX source code, right? Is there any licensing we should know about?

If @zjp is the author or otherwise holds the copyright, then by submitting it here they are agreeing to license it under QtPy's license, presuming that corporate contracts don't get in the way. Otherwise, there could theoretically be an issue, but while IANAL and this is not legal advice, the code is probably too short/simple to be covered by copyright protection as an independent work.

I'll add tests!

Looking forward to it!

@dalthviz dalthviz modified the milestones: v2.2.0, v2.2.1 Aug 1, 2022
@dalthviz dalthviz modified the milestones: v2.2.1, v2.3.0 Sep 23, 2022
@dalthviz dalthviz modified the milestones: v2.4.0, v2.4.1 Aug 11, 2023
@dalthviz dalthviz modified the milestones: v2.4.1, v2.5.0 Sep 1, 2023
@zjp zjp force-pushed the imagebytes branch 4 times, most recently from 3b15bd9 to b467e4d Compare April 18, 2024 20:40
@dalthviz
Copy link
Member

Thanks @zjp for giving this an update! Just in case, seems like the failing checks are related with coverallsapp/github-action#205

@zjp
Copy link
Contributor Author

zjp commented Apr 23, 2024

I still need to add a test for this function. Do you think it's better to add a small image to the repository, to fetch an image from the internet in the test, or to do a secret third thing?

@dalthviz
Copy link
Member

Do you think it's better to add a small image to the repository, to fetch an image from the internet in the test, or to do a secret third thing?

Maybe we could create a QImage manually? Like a black square, maybe even save it and then use that to validate the function? So creating an image with something like:

from qtpy.QtCore import QRectF, QSize, Qt
from qtpy.QtGui import QBrush, QImage, QPainter

image = QImage(QSize(100, 100), QImage.Format_RGB32)
painter = QPainter(image)
painter.setBrush(QBrush(Qt.black))
painter.fillRect(QRectF(0, 0, 100, 100), Qt.black)
image.save("black.png")

What do you think ? Also pinging @ccordoba12 just in case for more ideas

@ccordoba12
Copy link
Member

What do you think ? Also pinging @ccordoba12 just in case for more ideas

I like it, I think it's a good idea.

@zjp
Copy link
Contributor Author

zjp commented Apr 26, 2024

Sounds good to me.

@zjp
Copy link
Contributor Author

zjp commented Apr 26, 2024

I've added the test. It seemed sensible to me to test the length of the byte array, but if you immediately think of a better condition let me know.

@coveralls
Copy link

coveralls commented Jul 20, 2024

Coverage Status

coverage: 90.164% (+0.1%) from 90.016%
when pulling 7ebe5ee on zjp:imagebytes
into 0f7b181 on spyder-ide:master.

@zjp
Copy link
Contributor Author

zjp commented Jul 21, 2024

I was trying to fill in the color of the QImage in the test the wrong way, which led to a segfault (though curiously not on a couple platforms). Anyway, I think the QImage doesn't need to be filled in at all. A byte representing color will be the same length whether it's (1,1,1,1) or (0,0,0,0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants