BKE_lib_query: Add a partial ID usage iterator system. #121018

Merged
Bastien Montagne merged 5 commits from mont29/blender:tmp-libquery-subdata-foreachid into main 2024-04-28 17:35:13 +02:00

The idea is to allow iterating over e.g. all ID usages of a node from a
whole nodetree, using the same generic handling as existing 'whole ID'
foreach_id code.

This is necessary in some cases wher a sub-data needs to processed
independently from any 'owner ID', e.g. in some copy/paste handling.

This is a pre-requirement for proper fix of nodes copy/paste (see
e.g. #120103).

The idea is to allow iterating over e.g. all ID usages of a node from a whole nodetree, using the same generic handling as existing 'whole ID' `foreach_id` code. This is necessary in some cases wher a sub-data needs to processed independently from any 'owner ID', e.g. in some copy/paste handling. This is a pre-requirement for proper fix of nodes copy/paste (see e.g. #120103).
Bastien Montagne added 1 commit 2024-04-24 12:30:33 +02:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
a5c162dd40
WIP: BKE_lib_query: Add a partial ID usage iterator system.
The idea is to allow iterating over e.g. all ID usages of a node from a
whole nodetree, using the same generic handling as existing 'whole ID'
`foreach_id` code.

This is necessary in some cases wher a sub-data needs to processed
independently from any 'owner ID', e.g. in some copy/paste handling.

This is a pre-requirement for proper fix of nodes copy/paste (see
e.g. #120103).
Author
Owner

@blender-bot build

@blender-bot build
Bastien Montagne reviewed 2024-04-24 12:35:27 +02:00
@ -1420,0 +1424,4 @@
*
* See BKE_lib_query.hh and #IDTypeInfo.foreach_id for more details.
*/
void node_node_foreach_id(bNode *node, LibraryForeachIDData *data);
Author
Owner

Not very happy with this name, but since the node_ prefix is used everywhere when processing a whole nodetree...

Not very happy with this name, but since the `node_` prefix is used everywhere when processing a whole nodetree...
@ -0,0 +290,4 @@
}
return IDWALK_RET_NOP;
};
BKELibraryForeachSubdataID().operator()<bNode>(context.test_data.bmain,
Author
Owner

Not sure why, but the implicit template type deduction does not work here.

Makes me wonder if keeping the 'function call' operator is worth it, or if it would not be better to use a 'normal' template method.

Not sure why, but the implicit template type deduction does not work here. Makes me wonder if keeping the 'function call' operator is worth it, or if it would not be better to use a 'normal' template method.
Bastien Montagne requested review from Jacques Lucke 2024-04-24 12:36:01 +02:00
Bastien Montagne requested review from Julian Eisel 2024-04-24 12:36:01 +02:00
Bastien Montagne requested review from Hans Goudey 2024-04-24 12:36:02 +02:00
Author
Owner

All review is of course welcome, but am mostly looking for validation of the 'template' approach here, and/or for better ideas?

The idea of using a templated function (instead of just passing void * pointers around) is to improve type safety and such, but I was a bit disappointed to find out how much of a pain it was to get it to work... And I can't say I find current code nice & elegant at all.

All review is of course welcome, but am mostly looking for validation of the 'template' approach here, and/or for better ideas? The idea of using a templated function (instead of just passing `void *` pointers around) is to improve type safety and such, but I was a bit disappointed to find out how much of a pain it was to get it to work... And I can't say I find current code nice & elegant at all.
Member

I think it's possible to simplify this by just using a function:

void BKE_library_foreach_subdata(Main *bmain,
                                 ID *owner_id,
                                 ID *self_id,
                                 blender::FunctionRef<void(LibraryForeachIDData *)> foreach_id_fn,
                                 LibraryIDLinkCallback callback,
                                 void *user_data,
                                 int flag);

It's kind of unnecessary to have the template parameter in the current code, because self_id_subdata is only forwarded into subdata_foreach_id anyway. Instead, self_id_subdata could just be captured as part of the FunctionRef already.

I think it's possible to simplify this by just using a function: ``` void BKE_library_foreach_subdata(Main *bmain, ID *owner_id, ID *self_id, blender::FunctionRef<void(LibraryForeachIDData *)> foreach_id_fn, LibraryIDLinkCallback callback, void *user_data, int flag); ``` It's kind of unnecessary to have the template parameter in the current code, because `self_id_subdata` is only forwarded into `subdata_foreach_id` anyway. Instead, `self_id_subdata` could just be captured as part of the `FunctionRef` already.
Bastien Montagne force-pushed tmp-libquery-subdata-foreachid from a5c162dd40 to 9ba27dd216 2024-04-24 15:38:09 +02:00 Compare
Author
Owner

@JacquesLucke Not sure I understand your suggestion... How would the sub-data parameter be captured here?

I can see how to do it if using a 'wrapping' lambda, but this would be moving more work (and 'obfuscation' of code) to caller code, which I would rather avoid...

I can't seem to find any doc or example of how a FunctionRef itself could capture and forward parameters?

@JacquesLucke Not sure I understand your suggestion... How would the sub-data parameter be captured here? I can see how to do it if using a 'wrapping' lambda, but this would be moving more work (and 'obfuscation' of code) to caller code, which I would rather avoid... I can't seem to find any doc or example of how a `FunctionRef` itself could capture and forward parameters?
Member

I indeed mean the approach with the wrapping lambda. Seems like a more general solution. The current code expects a very specific signature of the node_node_foreach_id function, which also doesn't seem very obvious.

I think BKE_library_foreach_subdata (or similar) does not have to know about the exact sub-data that one wants to iterate over. E.g. I imagine that a single call to that function could be used to iterate over all IDs referenced by multiple nodes, or some other potentially more complex filter where a single pointer to the sub-data is not enough.

I indeed mean the approach with the wrapping lambda. Seems like a more general solution. The current code expects a very specific signature of the `node_node_foreach_id` function, which also doesn't seem very obvious. I think `BKE_library_foreach_subdata` (or similar) does not have to know about the exact sub-data that one wants to iterate over. E.g. I imagine that a single call to that function could be used to iterate over all IDs referenced by multiple nodes, or some other potentially more complex filter where a single pointer to the sub-data is not enough.
Bastien Montagne force-pushed tmp-libquery-subdata-foreachid from 9ba27dd216 to 9daceb581c 2024-04-26 12:02:06 +02:00 Compare
Author
Owner

@JacquesLucke Ah in that sens it makes sense... Updated the PR now.

@JacquesLucke Ah in that sens it makes sense... Updated the PR now.
Bastien Montagne changed title from WIP: BKE_lib_query: Add a partial ID usage iterator system. to BKE_lib_query: Add a partial ID usage iterator system. 2024-04-26 12:02:40 +02:00
Bastien Montagne force-pushed tmp-libquery-subdata-foreachid from 9daceb581c to 026357c3c7 2024-04-26 12:52:13 +02:00 Compare
Jacques Lucke approved these changes 2024-04-26 12:53:14 +02:00
Dismissed
Hans Goudey reviewed 2024-04-26 14:14:36 +02:00
@ -271,2 +271,4 @@
/**
* Loop over all of the ID's this data-block links to.
*
* @param bmain The Main data-base containing `owner_id`, may be null.
Member

@param bmain -> \param bmain:

`@param bmain` -> `\param bmain:`
Member

@param bmain -> \param bmain:
That's the syntax used elsewhere in Blender, might as well be consistent.

`@param bmain` -> `\param bmain:` That's the syntax used elsewhere in Blender, might as well be consistent.
mont29 marked this conversation as resolved
@ -278,1 +285,4 @@
int flag);
/**
* Apply `callback` to all ID usages of the data as dfined by `subdata_foreach_id`. Useful to e.g.
Member

dfined -> defined

`dfined` -> `defined`
mont29 marked this conversation as resolved
Bastien Montagne added 2 commits 2024-04-26 14:27:43 +02:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
ff9f42928d
Fixes from review.
Author
Owner

@blender-bot build

@blender-bot build
Jacques Lucke requested changes 2024-04-26 19:25:04 +02:00
Dismissed
@ -402,0 +411,4 @@
BLI_assert((flag & (IDWALK_RECURSE | IDWALK_DO_INTERNAL_RUNTIME_POINTERS |
IDWALK_DO_LIBRARY_POINTER | IDWALK_INCLUDE_UI)) == 0);
LibraryForeachIDData data;
Member

While testing #121122 I found that this is missing {} to zero initialize the data.
The issue was that cb_flag_clear is uninitialized which caused wrong behavior sometimes.

While testing #121122 I found that this is missing `{}` to zero initialize the data. The issue was that `cb_flag_clear` is uninitialized which caused wrong behavior sometimes.
Member

To clarify, LibraryForeachIDData data{}; is enough to default initialize everything in the struct.

To clarify, `LibraryForeachIDData data{};` is enough to default initialize everything in the struct.
mont29 marked this conversation as resolved
Bastien Montagne added 2 commits 2024-04-28 13:55:22 +02:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
ae30d3c359
Fix from review.
Bastien Montagne requested review from Jacques Lucke 2024-04-28 13:55:43 +02:00
Jacques Lucke approved these changes 2024-04-28 13:56:05 +02:00
Author
Owner

@blender-bot build

@blender-bot build
Bastien Montagne merged commit f933dae207 into main 2024-04-28 17:35:13 +02:00
Bastien Montagne deleted branch tmp-libquery-subdata-foreachid 2024-04-28 17:35:15 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#121018
No description provided.