When creating node groups, you can now enter different
images (and video as image). This patch extends support
for random images to include access to image information.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Is it possible to add focal length info to node. I think it is useful for camare control in the future.
Comment Actions
Potential malfunctions causing incorrect operation of the node
T99332: Regression: Resize video in image editor does not update correctly
T99340: IMB_anim_get_duration no valid after undo if animation is playing
Comment Actions
For frame and for animation
The main line is that the animation is one for the entire image.
Frames can be different size per frame index.
After a little deliberation, it was decided to divide the node into 2 modes of operation:
For frame and for animation
The main line is that the animation is one for the entire image.
Frames can be different size per frame index.
Comment Actions
I update to the master and enter the modes of operation. So far, an untested variant of conflict fixes.
Comment Actions
Yes, at the moment I upgraded to the master and returned the reviewers.
I still have some problems with the concept of locking in that the lock is needed not to gain access, but to loaded. That is, blocking and loading ensures that:
- Reading may be unsafe.
- Require its own blocking to read.
- Even after 3 hours after loaded and unlocking it will still be valide.
| source/blender/nodes/geometry/nodes/node_geo_image_info.cc | ||
|---|---|---|
| 88 | This overwrites image_user entirely, including the frames and framenr that was just set. | |
| 104 | Alpha seems like it should part of GEO_NODE_IMAGE_INFO_FRAME? It can change per frame. | |
| 110–117 | Unclear why this acquires the image buffer twice, once without an image user and once with. | |
| 122 | This returns without calling BKE_image_release_ibuf. | |
| 130 | BKE_image_has_anim is redundant as it checks if image->anims is empty, which you have already determined is not the case. | |
| 130 | If this only works for image->source == IMA_SRC_MOVIE, that check should be done earlier so that it does not try to acquire the image buffers or image anim at all when it's a different source. | |
| 131 | What is the purpose of checking BKE_image_has_loaded_ibuf? | |
| 131 | BKE_image_is_animated is redundant here since it contains ELEM(image->source, IMA_SRC_MOVIE, IMA_SRC_SEQUENCE). | |
| source/blender/nodes/NOD_static_types.h | ||
|---|---|---|
| 322 | ||
| source/blender/nodes/geometry/nodes/node_geo_image_info.cc | ||
| 24 | Suggestion: "Which frame to use for videos. Note that different frames in videos can have different resolutions" (I assume that's what this is meant to say) | |
| 28 | ||
| 31 | ||
| 33 | Fps -> FPS | |
| 37–48 | These functions are missing static | |
| 74 | I'd suggest using BLI_SCOPED_DEFER for this, should make the freeing more automatic. | |
| source/blender/nodes/geometry/nodes/node_geo_image_info.cc | ||
|---|---|---|
| 28 | This comment isn't done | |
| source/blender/nodes/geometry/nodes/node_geo_image_info.cc | ||
|---|---|---|
| 97 | Use static_cast in C++ code | |
Comment Actions
Looks much better, some comments that should be easy to address.
| source/blender/nodes/geometry/nodes/node_geo_image_info.cc | ||
|---|---|---|
| 30 | Can an image have a transparent channel -> Does the image have an alpha channel. We don't use the term "transparent channel" in Blender, the meaning of which would be unclear because alpha = 1 - transparency. | |
| 34 | This should be a float output, for example NTSC is 29.97, I don't think it's good to round that to an integer. | |
| 35 | speed frames per seconds -> speed in frames per second | |
| 38–47 | Remove this function and just add the following directly in node_geo_exec: params.set_output("Width", ibuf.x);
params.set_output("Height", ibuf.y);No need for some kind of validation of width/height or special handling of render resolution. | |
| 107 | Unclear to me why for !ianim we return early whereas for !ianim->anim we set default values frames=1 and fps=0. Seems more logical to set the default values always? | |
Comment Actions
I thought everything was fine, but I noticed that when the animation is played and undo, the frame number may stop returning
| source/blender/nodes/geometry/nodes/node_geo_image_info.cc | ||
|---|---|---|
| 64 | Do not throw an exception. Failing to load an image from disk should not crash Blender. | |
| source/blender/nodes/geometry/nodes/node_geo_image_info.cc | ||
|---|---|---|
| 64 | I decided to add this by analogy with another node. {https://developer.blender.org/diffusion/B/browse/master/source/blender/nodes/geometry/nodes/node_geo_image_texture.cc$70} | |
Comment Actions
That node implementation catches the exception.
Just remove the exception throwing code in this patch.
Comment Actions
Oh yeah, I didn't study that node well enough to catch the exception. Yes. Since other nodes do not seem to report problems with image data in the interface or console, this check is also redundant here.
Comment Actions
To be honest, I started this node after seeing simple size getter functions. As a result, I had to do this a couple of times since the beginning, as it turned out to be very useful and desirable. Due to the fact that there is little code, and I was not familiar with the image api before starting this, the result looks like 4 lines of code to one multi-line comment ... I hope I don't get into this game again )