Changeset View
Standalone View
source/blender/nodes/geometry/nodes/node_geo_image_info.cc
- This file was added.
| /* SPDX-License-Identifier: GPL-2.0-or-later */ | ||||||||||
| #include "BKE_image.h" | ||||||||||
| #include "BLI_path_util.h" | ||||||||||
| #include "IMB_colormanagement.h" | ||||||||||
| #include "IMB_imbuf.h" | ||||||||||
| #include "IMB_imbuf_types.h" | ||||||||||
| #include "UI_interface.h" | ||||||||||
| #include "UI_resources.h" | ||||||||||
| #include "node_geometry_util.hh" | ||||||||||
| namespace blender::nodes::node_geo_image_info_cc { | ||||||||||
| static void node_declare(NodeDeclarationBuilder &b) | ||||||||||
| { | ||||||||||
| b.add_input<decl::Image>(N_("Image")).hide_label(); | ||||||||||
| b.add_input<decl::Int>(N_("Frame")) | ||||||||||
| .min(0) | ||||||||||
| .description(N_("Which frame to use for videos. Note that different frames in videos can " | ||||||||||
brecht: I think "Has Alpha" would be a better name, the image buffer has alpha or not, it's not about… | ||||||||||
| "have different resolutions")); | ||||||||||
Done Inline ActionsSuggestion: "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) HooglyBoogly: Suggestion: `"Which frame to use for videos. Note that different frames in videos can have… | ||||||||||
Done Inline ActionsMissing N_. Same below. JacquesLucke: Missing `N_`. Same below. | ||||||||||
| b.add_output<decl::Int>(N_("Width")); | ||||||||||
| b.add_output<decl::Int>(N_("Height")); | ||||||||||
| b.add_output<decl::Bool>(N_("Has Alpha")) | ||||||||||
Done Inline Actions
HooglyBoogly: | ||||||||||
Done Inline ActionsThis comment isn't done HooglyBoogly: This comment isn't done | ||||||||||
| .description(N_("Whether the image has an alpha channel")); | ||||||||||
Done Inline ActionsIt's not clear to me why these sockets have different UI names and identifiers. brecht: It's not clear to me why these sockets have different UI names and identifiers. | ||||||||||
Done Inline ActionsCan 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. brecht: `Can an image have a transparent channel` -> `Does the image have an alpha channel`.
We don't… | ||||||||||
| b.add_output<decl::Int>(N_("Frame Count")) | ||||||||||
Done Inline Actions
HooglyBoogly: | ||||||||||
| .description(N_("The number of animation frames. If a single image, then 1")); | ||||||||||
| b.add_output<decl::Float>(N_("FPS")).description( | ||||||||||
Done Inline ActionsFps -> FPS HooglyBoogly: `Fps` -> `FPS` | ||||||||||
| N_("Animation playback speed in frames per second. If a single image, then 0")); | ||||||||||
Done Inline ActionsThis should be a float output, for example NTSC is 29.97, I don't think it's good to round that to an integer. brecht: This should be a float output, for example NTSC is `29.97`, I don't think it's good to round… | ||||||||||
| } | ||||||||||
Done Inline Actionsspeed frames per seconds -> speed in frames per second brecht: `speed frames per seconds` -> `speed in frames per second` | ||||||||||
| static void node_geo_exec(GeoNodeExecParams params) | ||||||||||
| { | ||||||||||
Done Inline Actionsshould be static JacquesLucke: should be `static` | ||||||||||
| Image *image = params.get_input<Image *>("Image"); | ||||||||||
| const int frame = params.get_input<int>("Frame"); | ||||||||||
| if (!image) { | ||||||||||
| params.set_default_remaining_outputs(); | ||||||||||
| return; | ||||||||||
| } | ||||||||||
| ImageUser image_user; | ||||||||||
| BKE_imageuser_default(&image_user); | ||||||||||
Done Inline ActionsRemove 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. brecht: Remove this function and just add the following directly in `node_geo_exec`:
```
params. | ||||||||||
| image_user.frames = INT_MAX; | ||||||||||
Done Inline ActionsThese functions are missing static HooglyBoogly: These functions are missing `static` | ||||||||||
| image_user.framenr = BKE_image_is_animated(image) ? frame : 0; | ||||||||||
| void *lock; | ||||||||||
| ImBuf *ibuf = BKE_image_acquire_ibuf(image, &image_user, &lock); | ||||||||||
| BLI_SCOPED_DEFER([&]() { BKE_image_release_ibuf(image, ibuf, lock); }); | ||||||||||
| if (!ibuf) { | ||||||||||
| params.set_default_remaining_outputs(); | ||||||||||
| return; | ||||||||||
| } | ||||||||||
Done Inline ActionsShould this be 1 for still frames? brecht: Should this be 1 for still frames? | ||||||||||
| params.set_output("Has Alpha", ELEM(ibuf->planes, 32, 16)); | ||||||||||
| params.set_output("Width", ibuf->x); | ||||||||||
| params.set_output("Height", ibuf->y); | ||||||||||
| int frames = 1; | ||||||||||
| float fps = 0.0f; | ||||||||||
Done Inline ActionsDo not throw an exception. Failing to load an image from disk should not crash Blender. brecht: Do not throw an exception. Failing to load an image from disk should not crash Blender. | ||||||||||
Done Inline ActionsI 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} Moder: I decided to add this by analogy with another node. {https://developer.blender. | ||||||||||
| if (ImageAnim *ianim = static_cast<ImageAnim *>(image->anims.first)) { | ||||||||||
| auto *anim = ianim->anim; | ||||||||||
| if (anim) { | ||||||||||
| frames = IMB_anim_get_duration(anim, IMB_TC_NONE); | ||||||||||
| short fps_sec = 0; | ||||||||||
| float fps_sec_base = 0.0f; | ||||||||||
| IMB_anim_get_fps(anim, &fps_sec, &fps_sec_base, true); | ||||||||||
| fps = float(fps_sec) / fps_sec_base; | ||||||||||
Done Inline ActionsI'd suggest using BLI_SCOPED_DEFER for this, should make the freeing more automatic. HooglyBoogly: I'd suggest using `BLI_SCOPED_DEFER` for this, should make the freeing more automatic. | ||||||||||
| } | ||||||||||
| } | ||||||||||
| params.set_output("Frame Count", frames); | ||||||||||
| params.set_output("FPS", fps); | ||||||||||
| } | ||||||||||
| } // namespace blender::nodes::node_geo_image_info_cc | ||||||||||
| void register_node_type_geo_image_info() | ||||||||||
| { | ||||||||||
| namespace file_ns = blender::nodes::node_geo_image_info_cc; | ||||||||||
| static bNodeType ntype; | ||||||||||
Done Inline ActionsThis overwrites image_user entirely, including the frames and framenr that was just set. brecht: This overwrites `image_user` entirely, including the `frames` and `framenr` that was just set. | ||||||||||
| geo_node_type_base(&ntype, GEO_NODE_IMAGE_INFO, "Image Info", NODE_CLASS_INPUT); | ||||||||||
| ntype.declare = file_ns::node_declare; | ||||||||||
| ntype.geometry_node_execute = file_ns::node_geo_exec; | ||||||||||
| node_type_size_preset(&ntype, NODE_SIZE_LARGE); | ||||||||||
| nodeRegisterType(&ntype); | ||||||||||
| } | ||||||||||
Done Inline ActionsUnclear why this acquires the image buffer twice, once without an image user and once with. brecht: Unclear why this acquires the image buffer twice, once without an image user and once with. | ||||||||||
Done Inline ActionsAlpha seems like it should part of GEO_NODE_IMAGE_INFO_FRAME? It can change per frame. brecht: Alpha seems like it should part of `GEO_NODE_IMAGE_INFO_FRAME`? It can change per frame. | ||||||||||
Done Inline ActionsThis returns without calling BKE_image_release_ibuf. brecht: This returns without calling `BKE_image_release_ibuf`. | ||||||||||
Done Inline ActionsWhat is the purpose of checking BKE_image_has_loaded_ibuf? brecht: What is the purpose of checking `BKE_image_has_loaded_ibuf`? | ||||||||||
Done Inline ActionsBKE_image_is_animated is redundant here since it contains ELEM(image->source, IMA_SRC_MOVIE, IMA_SRC_SEQUENCE). brecht: `BKE_image_is_animated` is redundant here since it contains `ELEM(image->source, IMA_SRC_MOVIE… | ||||||||||
Done Inline ActionsBKE_image_has_anim is redundant as it checks if image->anims is empty, which you have already determined is not the case. brecht: `BKE_image_has_anim` is redundant as it checks if `image->anims` is empty, which you have… | ||||||||||
Done Inline ActionsIf 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. brecht: If this only works for `image->source == IMA_SRC_MOVIE`, that check should be done earlier so… | ||||||||||
Done Inline ActionsUse static_cast in C++ code HooglyBoogly: Use `static_cast` in C++ code | ||||||||||
Done Inline ActionsUnclear 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? brecht: Unclear to me why for `!ianim` we return early whereas for `!ianim->anim` we set default values… | ||||||||||
I think "Has Alpha" would be a better name, the image buffer has alpha or not, it's not about the file format supporting alpha or something like that.
I also would not make this the first socket, doesn't seem that important.