Geometry Nodes: support materials in simulation state with data-block mapping #109703
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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
6 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#109703
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "JacquesLucke/blender:geometry-nodes-id-mapping"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This implements the design proposed in #108410 to support materials in the simulations.
The task also describes why this is not a trivial issue.
The solution is to have an explicit
name (+ library name) -> data block
mapping that is stored in the modifier. Thelibrary name
is necessary for it to be a unique identifier within a .blend file. The baked data only contains names of the used materials. When the baked data is loaded, the correct material is looked up from the mapping.The most tricky aspect here is to decide when the mapping is created. There are different possibilities like forcing the user to create it manually, updating it automatically after specific user actions, or creating it automatically during depsgraph evaluation whenever a missing mapping is detected.
Currently, the last option is used even though it is quite unconventional, because it leads to the best out-of-the-box user experience. The main difficulties are:
Possible further developments:
Possible TODOs for this patch:
@ -1381,2 +1467,2 @@
"internal_dependencies",
N_("Internal Dependencies"),
"id_mappings",
N_("ID Mappings"),
In the UI, these should be referred to as "Data Blocks" rather than "IDs".
ID
is just the name of the struct in the code, and it's not even a great name :PWIP: Geometry Nodes: support materials in simulation state with id mappingto Geometry Nodes: support materials in simulation state with data-block mappingIn general this makes sense I think, noticed some things to change inline though.
Could the bake operator create the mapping maybe?
@ -40,9 +40,21 @@ class DATA_PT_gpencil_modifiers(ModifierButtonsPanel, Panel):
layout.template_grease_pencil_modifiers()
class DATA_UL_nodes_modifier_id_mappings(UIList):
Better to define this in the modifier file like
MOD_gpencil_legacy_dash.c
.@ -377,0 +385,4 @@
IDProperty *materials_prop = IDP_NewIDPArray(".materials");
for (const int i : io_materials->elements().index_range()) {
IDPropertyTemplate idprop = {0};
IDProperty *material_prop = IDP_New(IDP_GROUP, &idprop, std::to_string(i).c_str());
A bit nicer IMO:
bke::idprop::create_group(std::to_string(i)).release()
@ -377,0 +388,4 @@
IDProperty *material_prop = IDP_New(IDP_GROUP, &idprop, std::to_string(i).c_str());
const DictionaryValue *io_material = io_materials->elements()[i]->as_dictionary_value();
if (io_material != nullptr) {
How about this?
@ -377,0 +398,4 @@
}
IDP_AppendArray(materials_prop, material_prop);
/* IDP_AppendArray does a shallo copy. */
shallo
->shallow
@ -3719,1 +3723,4 @@
/** \} */
/* ------------------------------------------------------------------- */
/** \name Update id mapping in geometry nodes modifier
Section titles are title case
@ -3720,0 +3726,4 @@
/** \name Update id mapping in geometry nodes modifier
* \{ */
static ID *find_id_by_names(Main &bmain,
Maybe this should be moved elsewhere? Seems like it might be useful elsewhere, and that would help clarify there's nothing modifier specific here
@ -3720,0 +3727,4 @@
* \{ */
static ID *find_id_by_names(Main &bmain,
const blender::StringRef &id_name,
Pass
StringRef
by value, right?@ -3720,0 +3775,4 @@
nmd.id_mappings = static_cast<NodesModifierIDMapping *>(
MEM_reallocN(nmd.id_mappings, sizeof(NodesModifierIDMapping) * new_mappings_num));
NodesModifierIDMapping *new_mappings = nmd.id_mappings + nmd.id_mappings_num;
int new_mapping_i = nmd.id_mappings_num;
That's a bit nicer than incrementing a raw pointer IMO
@ -3720,0 +3831,4 @@
Main *bmain = CTX_data_main(C);
Object *ob = ED_object_active_context(C);
ModifierData *md = BKE_object_active_modifier(ob);
These operators should probably use the
"modifier"
context property rather than the active modifier, so they work properly when interacting with two geometry nodes modifiers.See the
Generic Invoke Functions
section in this file for some utilities that help with that.@ -3720,0 +3840,4 @@
if (nmd.active_id_mapping < 0 || nmd.active_id_mapping >= nmd.id_mappings_num) {
return OPERATOR_CANCELLED;
}
NodesModifierIDMapping &active_mapping = nmd.id_mappings[nmd.active_id_mapping];
I wonder if some of this boilerplate could be removed using something like
std::remove
if theNodesModifierIDMapping
had at least some RAII semantics. That could probably help elsewhere too, and maybe it's a good habit to get into.I think using RAII for raw DNA structs is more trouble than it's worth currently. Mainly because we have a fair amount of code that deals with DNA structs as raw memory. It would be nice to use RAII for DNA types (or in wrapper types) in the future, but I think we should have a more global design before starting with that.
@ -3720,0 +3846,4 @@
id_us_min(active_mapping.id);
active_mapping.id = nullptr;
memmove(nmd.id_mappings + nmd.active_id_mapping,
I notice this doesn't reallocate the array. Maybe imaginary problem, but I'm thinking of a case where there were 1000 mappings and all are removed, and the memory is still used.
@ -7057,0 +7095,4 @@
RNA_define_lib_overridable(true);
prop = RNA_def_property(srna, "id_name", PROP_STRING, PROP_NONE);
RNA_def_property_ui_text(prop, "ID Name", "Name of the ID that is to be mapped");
ID -> Data-Block
Same below
@ -7057,0 +7100,4 @@
prop = RNA_def_property(srna, "lib_name", PROP_STRING, PROP_NONE);
RNA_def_property_ui_text(
prop, "Library Name", "Name of the library that contains the ID that is to be mapped");
ID that is to be mapped
->mapped data-block
Gets the same thing across less awkwardly I think
@ -355,2 +355,4 @@
},
&settings);
for (const int i : IndexRange(nmd->id_mappings_num)) {
Same in other places, where possible IMO
@ -144,6 +147,35 @@ static void remove_materials(Material ***materials, short *materials_num)
*materials_num = 0;
}
static void store_materials_as_id_properties(ID &id)
Not really a new problem, but I wonder if it's worth adding some error reporting here. Only issue is that the geo nodes exec params would have to be passed this deep, which is pretty ugly. So maybe not...
I listed that in the "Possible further developments" section in the PR description. Note that one does not have to pass
GeoNodeExecParams
here to be able to create a warning. The logger can be retrieved fromGeoNodesLFLocalUserData
.Ah, sorry I missed that
@ -179,0 +264,4 @@
bke::BakeIDMappingIssuesLog *id_mapping_issues)
{
geometry_set.modify_geometry_sets([&](GeometrySet &geometry) {
if (Mesh *mesh = geometry.get_mesh_for_write()) {
Not sure if it's worth checking if there are materials to restore before retrieving write access, I guess if that is actually a performance problem we aren't using implicit sharing enough ;)
Yeah, wouldn't bother with that for now. The mesh needs to be retrieved for writing for renaming anonymous attributes as well.
Only when there are anonymous attributes that need to be renamed now. But yes, right :)
@ -176,0 +173,4 @@
/**
* All simulation states, sorted by frame.
*/
Vector<std::unique_ptr<ModifierSimulationStateAtFrame>> states_at_frames_;
This looks like a merge error
@ -580,3 +619,2 @@
static std::shared_ptr<io::serialize::ArrayValue> serialize_material_slots(
const Span<const Material *> material_slots)
static void serialize_material_slots(DictionaryValue &io_dict, const IDProperty *id_properties)
What do you think about generalizing this to write all IDProperties? Seems like that would actually simplify this code, and we'd get that feature of not losing custom properties for free too
Generally a nice idea. We already have code for that too (
convert_to_serialize_values
). Unfortunately, it seems to store a fair amount of additional data that we don't really need here: https://archive.blender.org/developer/D12990Not sure if we should bloat up the json data when it doesn't offer functional benefits for materials.
Some changes to existing code would be necessary as well, because it does not support
IDP_IDARRAY
.@ -3720,0 +3814,4 @@
/** \} */
/* ------------------------------------------------------------------- */
/** \name Remove id mapping in geometry nodes modifier
Title case
@ -3720,0 +3882,4 @@
/** \} */
/* ------------------------------------------------------------------- */
/** \name Add id mapping in geometry nodes modifier
Title case
@ -2326,6 +2326,14 @@ typedef struct NodesModifierSettings {
struct IDProperty *properties;
} NodesModifierSettings;
typedef struct NodesModifierIDMapping {
Might as well mention how this corresponds to
BakeIDMappingKey
here, or maybe the other way around.@ -2335,2 +2343,4 @@
*/
char *simulation_bake_directory;
int id_mappings_num;
Add a high-level comment here:
Or something like that :)
@ -7057,0 +7108,4 @@
RNA_def_property_flag(prop, PROP_EDITABLE | PROP_ID_SELF_CHECK);
RNA_def_property_pointer_funcs(
prop, nullptr, nullptr, "rna_NodesModifier_id_mapping_id_typef", nullptr);
RNA_def_property_ui_text(prop, "ID", "");
Data-Block
@ -7057,0 +7141,4 @@
prop = RNA_def_property(srna, "active_index", PROP_INT, PROP_NONE);
RNA_def_property_int_sdna(prop, nullptr, "active_id_mapping");
RNA_def_property_ui_text(prop, "Active ID Mapping Index", "");
Data-Block
@ -1405,0 +1483,4 @@
RNA_pointer_create(
ptr->owner_id, &RNA_NodesModifierIDMapping, &active_mapping, &active_mapping_ptr);
uiTemplateAnyID(col, &active_mapping_ptr, "id", "id_type", "ID");
Based on the screenshot, this is missing
Just adding this looks a bit bad in the ID row with the extra type dropdown unfortunately.
Maybe the dropdown should be moved to a separate line, or hidden for now since we only use this for materials currently.
Yeah,
uiTemplateAnyID
looks a bit ancient, it was never updated for the property split change. I think I'd recommend just taking the equivalent of the twouiItemFullR
calls in there. I started messing with that but didn't quite finish@ -1475,2 +1574,4 @@
IDP_BlendDataRead(reader, &nmd->settings.properties);
}
BLO_read_data_address(reader, &nmd->id_mappings);
for (const NodesModifierIDMapping &mapping : MutableSpan(nmd->id_mappings, nmd->id_mappings_num))
const NodesModifierIDMapping
->NodesModifierIDMapping
I think?Yeah, C casts are fun :D
That can help, but I wonder if this implicit behavior is too much magic or not...
If we can make it so that people don't have to deal with this list UI at all, we should take that opportunity IMO. This is the sort of thing people expect to "just work" unless they're aware of the implementation details.
@ -7057,0 +7113,4 @@
prop = RNA_def_property(srna, "id_type", PROP_ENUM, PROP_NONE);
RNA_def_property_enum_items(prop, rna_enum_id_type_items);
RNA_def_property_enum_default(prop, ID_MA);
This property probably shouldn't be animate-able
@ -1405,0 +1488,4 @@
ptr->owner_id, &RNA_NodesModifierIDMapping, &active_mapping, &active_mapping_ptr);
uiTemplateAnyID(col, &active_mapping_ptr, "id", "id_type", "ID");
uiItemR(col, &active_mapping_ptr, "id_name", 0, "ID Name", ICON_NONE);
Heh, "ID" is hard to get rid of! Here too
Also, don't forget
IFACE_
is necessary in UI code when manually specifying a labelI kind of agree with Hans that ideally in most common cases the user shouldn't have to touch or even know about the existence of this panel. It feels really powerful and I like how this translates into remapping any possible datablock. But it really feels like it shouldn't be a necessity to set this up for common use-cases.
I'm not sure what the proper solution would be, but it seems like the update button is working quite well. Would it be an option to have a toggle for a 'coupled' mode that would run the operator automatically when the data isn't baked? Then you could still turn that off to have all the manual control.
Not sure what "coupled" means here exactly. If we want to automate this more, the main question for me is, what user actions should trigger the refresh of the mapping? It feels like it should be a well defined set of operators that also update the mapping, instead of running it all the time magically.
I just used that for lack of a better word. I guess auto-sync would be more accurate.
That's the tricky part. Off the top of my head I don't have that list. Ideally it would just work consistently whenever the list of datablocks referenced by the simulation data changes. But this could even happen at any time throughout the simulation dynamically right? And I don't think there would be a way to distinguish those types of changes.
I'm not sure how exactly the update operator works and how smart it is, but in theory this would need to run on all kinds of operations with all dependencies of the object to be safe, right?
Correct. The operator works by using logged data from the previous evaluations of the node tree. During evaluation the simulation zone logs the name of materials that were used. That list is stored as a cache internally. When the refresh button is pressed, the mappings for the names in the list are created.
I'm not sure whether you think it would be ok to if we just show to the user when data blocks are missing. For example, the node could just have a warning. Would be nice if the warning could also have a button for the refresh, but not sure how that would look like right now.
That logic sounds pretty sensible and solid.
But if this information is tracked anyways, what's the issue with just running the update whenever this warning would show up automatically? So whenever datablocks go missing, just update. If the missing data cannot be resolved, show the warning. And then making manual changes would require the user to turn off the auto-sync option.
Could that work?
The problem is described in #108410. It's a very unconventional thing to change data like this on original objects, while the depsgraph is evaluated. Also not sure how that would work with undo. Not to mention that adding the mapping would require the depsgraph to be evaluated again after the materials are all added to it. Tagging the depsgraph during evaluation is something we probably don't do anywhere yet.
Okay, I see. Hmm, I do feel quite strongly that the user shouldn't have to interact with this in most cases. But I don't have a solution to that either. Let's discuss this more tomorrow, as this really is something that should be figured out for 4.0 ...
I showed this to @Sergey
He mentioned that this seems like it's very close to implementing something like layering as it exists in USD, so there might be some synergy there, should probably be done in a consistent way.
Also maybe for @brecht
In terms of depsgraph, he said it's not immediately clear why you would have to trigger a rebuild on an update, as all the dependencies should already be part of the geometry at that point, right?
He also proposed to nudge down the amount of low level user access, by not showing the db and lib name as editable but read-only.
Could be, don't know what's going on there exactly.
They probably are in the depsgraph already, yes. Still, the depsgraph would be out of date because it's missing some relations. I'm not aware of any other situation where this would be the case. Also, it's not entirely clear to me if the mapping should also be created from non-active depsgraph like for rendering.
I think being able to edit those is quite important in general when things have been renamed and you still want to use an existing cache. Not sure if we can make them read-only by default, or if that would really help.
Can't really speak on the first 2 topics.
Maybe I'm wrong, but shouldn't that still work? You would rename the datablock in the file, not in the cache itself. If it's baked you don't touch the list itself, but the assigned datablock gets renamed. I guess if it's not baked you would need to update the name to keep it up to date and deduplicate with frames that are yet to be cached (?). But it wouldn't be terrible if in this case there are 2 entries mapping to the same datablock imo.
But maybe I'm missing something (?)
Yes, I think it would work fine for the large majority of use cases we run into nowadays. I'm more concerned about the case when we load baked data that has been baked independently. Guess that's less of a use case so far, because we don't have the Import Bake node (or other import nodes for that matter) yet. I'm fine with making them read-only for now, and worry about that again in the future. I'm not entirely sure what we win by making them read-only instead of leaving them editable or just graying them out.
To me there are two separate aspects here:
Only the latter step is where there seems to be overlap with what one expects from USD and layering integration, and where some alignment between teams could be very good. I am not sure if this is something considered an integral part of the caching project which is aimed to be solved for Blender 4.0.
If the scope can be narrowed to only preservation ID assignment throughout the cache then I am not sure why we need to go into this user level complexity. The library path + ID name is sufficient to identify data-block within a .blend file. So storing them in the cache should suffice for the purpose of mapping.
Surely material can be renamed, but then the cache technically becomes out-dated. And to me the recovery from this is to re-cache instead of going to some editor and re-assigning materials in the mapping.
The only case when it will happen is if the cache is read is a different .blend file which does not contain the entire node tree. I do not know if this is the intended use of the system at this time.
If the cache is consistent with the current state of the tree then all the relations should already be in the dpesgrph. If the cache is not consistent with the state of the tree then it should be re-cached.
I think it's mainly about presenting the relevant information to the user in a way that is least confusing and without committing to this being the way that we handle overriding/layering data more generically.
I'm fine with ignoring the use case of overriding used IDs for now.
I don't quite agree with that statement. If you bake something, then you should still be able to use the baked data even if some of the inputs to the simulation changed. Until then, loading the baked data including making links to data blocks should work. It's up to the user when he wants to bake again. Otherwise, all many tiny changes in .blend files will make bakes invalid.
Also, just looking up IDs by name and library name during depsgraph evaluation seems wrong to me, especially if we can't be 100% sure that the data block is in the depsgraph and it's evaluated before the current object (which would be the case if we just use the name strings stored in the baked data). Are we doing that anywhere else?
I'm not entirely sure if you (@Sergey) were proposing not to store the mapping in the modifier explicitly, but to do something else instead, or if you were just arguing that we don't need to give the user the ability to change the mapping names yet.
We discussed this in a meeting. My notes.
A concern that came up when talking about this with @SimonThommes. Is the mapping per cache/bake? If so, for multiple simulation zones that would cause problems for auto updating. Since you wouldn't be able to clear the whole cache when e.g. one simulation zone is baked and the other is getting cleared.
For the UI, the layout is a bit confusing to me as part of the "Internal Dependencies" panel. I would expect this to be part of a Bake/Cache panel, maybe a subpanel named "Referenced Data-blocks" or "Used Data-blocks" or something like that. From looking at the screenshot in the description, it wouldn't be obvious to me that this is only about simulation and caching/baking.
All of that sounds reasonable to me, except for the sentence below which is ambiguous (it's missing
is
oris not
).I addressed this in the original patch description already and also just talked about this with Simon.
Basically, I think it's totally reasonable to store the mapping at different levels depending on the use case, but I think storing it on the modifier is a reasonable way to start.
I just noticed one issue we didn't talk about before. When adding the mapping on original data during depsgraph evaluation, I also have to increase the user count (
id_us_plus
). However, doing that is not thread-safe. Will just ignore this race condition for now and keep going, but it's something to keep in mind.Generally for me this is working as expected. Testing this a bit more in-depth, the ability to manually change this mapping has some further implications that are not immediately clear.
I have a few proposals:
Cache Data Block Mapping
Update
orClear
button that gets rid of all elements and rebuild the list using the automatic functionality.All in all, considering everything , this might be too much to still validate properly for 4.0 in the scope currently aimed at.
In my personal opinion, for 4.0 it would already be useful in a state where you cannot edit this list at all. Then it's always just a reflection of what has been cached and Blender manages it autonomously. This would still work fine in most common cases and in failing cases there is at least some insight why you might need to re-cache.
But that would rely on always keeping the list in sync with the data blocks that are actually being used.
What do you think?
We are in bcon3 already. This won't be part of 4.0.
Yes, mentioned that in the patch description already.
Automatically removing mappings is quite a bit more challenging in general. We can still make some useful utilities though. One also has to be much more careful with automatically deleting data than with creating data, even if the data that is deleted was generated.
Ah sorry, I missed your update to that.
Hi, is this going to be re-implemented before geo-node development takes a back seat? just i see people need this!? it would be a positive for the community and for geo-nodes to move ahead with proponents being able to utilise it.
Jacques Lucke referenced this pull request2024-01-11 20:24:41 +01:00
Closing this in favor of #117043.
Pull request closed