Closed Gigahawk closed 2 years ago
Hello @Gigahawk and thanks for that PR ! Offscreen rendering was something I had in mind for a while ... but never took time to implement. Thanks for having taken the time to do it. I'm starting to review it, then I will give you my mind about it :)
Here are some fixes to make this feature more sound with the rest of the rendering API :)
I systematically packed width,height informations as they are not independent things: They had to go together, so a tuple
or a uvec2
seems better.
I'm wondering what is actually the best as return type for an image: a PIL Image
or a numpy ndarray
? :thinking: The second is universal and tho closest to the mathematical reality of an image. The first gives information on what subpixel is placed where, but following RGBA convention is quite common now, so having a numpy array would not be that ambiguous. Also the first provide the methods save
and show
that could be convenient ...
There is still things to clarify about the methods in ViewCommon
: they are currently using QPoint
and QPointF
as argument/return types, because they where initially used exclusively in a QWidget
. But now that they are placed in a common class (hence not Qt oriented), we should use different types.
I have to think and decide on this point
- I'm wondering what is actually the best as return type for an image: a PIL
Image
or a numpyndarray
? :thinking: The second is universal and tho closest to the mathematical reality of an image. The first gives information on what subpixel is placed where, but following RGBA convention is quite common now, so having a numpy array would not be that ambiguous. Also the first provide the methodssave
andshow
that could be convenient ...
Personally I prefer a PIL Image, since for my current application I'm doing further processing in PIL to the output. I'm not sure I agree that numpy is the correct way to store an image. An array is suited for a blob of some generic data, but here the output is specifically a picture, where PIL provides lots of functionality that is relevant for working with pictures.
- There is still things to clarify about the methods in
ViewCommon
: they are currently usingQPoint
andQPointF
as argument/return types, because they where initially used exclusively in aQWidget
. But now that they are placed in a common class (hence not Qt oriented), we should use different types. I have to think and decide on this point
Can't these just be changed to use a tuple or vector? I didn't bother touching them cause I wasn't really sure what they did and it seemed to work fine. But either those functions are only required when the image is being rendered to a QWidget, or they're actually common across different render targets and can just use some generic type
Can't these just be changed to use a tuple or vector?
The sad thing with Qt is that we cannot just pass it a generic type like a tuple These methods are required for a headless view I think, or it doesn't make sense to call it a headless view and put it a navigation and a projection object. for pure rendering, two fixed matrices and no methods at all would be sufficient.
Also I made my mind on the qt vs glm types for methods: for now
QPoint
as interface types for View
ivec2
as interface type for Headless
Anything you want to add to this PR before I merge ?
Can't these just be changed to use a tuple or vector?
The sad thing with Qt is that we cannot just pass it a generic type like a tuple These methods are required for a headless view I think, or it doesn't make sense to call it a headless view and put it a navigation and a projection object. for pure rendering, two fixed matrices and no methods at all would be sufficient.
Also I made my mind on the qt vs glm types for methods: for now
- I will keep
QPoint
as interface types forView
- but I will use
ivec2
as interface type forHeadless
Anything you want to add to this PR before I merge ?
That's unfortunate, but what you propose sounds reasonable.
I'm done with this PR for now, feel free to merge.
Running![kinematic](https://user-images.githubusercontent.com/10356230/149288022-54a9325a-0c0f-477b-99d4-65f7818a869f.png)
examples/kinematic.py
should produce the following: