Fix: Simplify "Volume Resolution" not updating for non volume objects #117848

Philipp Oeser · 2024-02-05T14:35:26+01:00

Philipp Oeser commented

2024-02-05 14:35:26 +01:00

This came up in #117747.

Unless an object was of OB_VOLUME type, the setting was not updating the
viewport accordingly since the introduction in 365bf103d1. So if for
example a mesh has a "Mesh to Volume" node, the update did not take
place.

If we use context, we can check the evaluated GeometrySet on any object
type with a geometry nodes modifier though and if it has a volume, we
can also tag for update.

This came up in #117747. Unless an object was of OB_VOLUME type, the setting was not updating the viewport accordingly since the introduction in 365bf103d1f6. So if for example a mesh has a "Mesh to Volume" node, the update did not take place. If we use context, we can check the evaluated GeometrySet on any object type with a geometry nodes modifier though and if it has a volume, we can also tag for update.

Philipp Oeser added 1 commit 2024-02-05 14:35:36 +01:00

95ee1a46d7 Fix: Simplify "Volume Resolution" not updating for non volume objects

This came up in #117747.

Unless an object was of OB_VOLUME type, the setting was not updating the
viewport accordingly since the introduction in 365bf103d1. So if for
example a mesh has a "Mesh to Volume" node, the update did not take
place.

If we use context, we can check the evaluated GeometrySet on any object
type with a geometry nodes modifier though and if it has a volume, we
can also tag for update.

Philipp Oeser requested review from Jacques Lucke 2024-02-05 14:35:53 +01:00

Philipp Oeser added this to the Nodes & Physics project 2024-02-05 14:36:00 +01:00

Hans Goudey reviewed 2024-02-05 14:38:54 +01:00

Hans Goudey left a comment

Checking the evaluated geometry set isn't enough either, since a volume might be an intermediate step in node evaluation that isn't included in the result.

Philipp Oeser commented

2024-02-05 14:44:55 +01:00

Checking the evaluated geometry set isn't enough either, since a volume might be an intermediate step in node evaluation that isn't included in the result.

Hm, OK, thx checking. So is there a GeometrySet that represents the final output?

> Checking the evaluated geometry set isn't enough either, since a volume might be an intermediate step in node evaluation that isn't included in the result. Hm, OK, thx checking. So is there a `GeometrySet` that represents the final output?

Hans Goudey commented

2024-02-05 14:52:18 +01:00

geometry_set_eval represents the final output, that's right. But my point was that volumes might be used internally (in a way that should respect the simplify setting) but not be included in the final output.

`geometry_set_eval` represents the final output, that's right. But my point was that volumes might be used internally (in a way that should respect the simplify setting) but not be included in the final output.

Philipp Oeser commented

2024-02-05 14:59:56 +01:00

geometry_set_eval represents the final output, that's right. But my point was that volumes might be used internally (in a way that should respect the simplify setting) but not be included in the final output.

Ah, OK. Not sure if we should really consider the internal usage when it is not the final output (since this is a viewport setting at least I would not expect it to)

> `geometry_set_eval` represents the final output, that's right. But my point was that volumes might be used internally (in a way that should respect the simplify setting) but not be included in the final output. Ah, OK. Not sure if we should really consider the internal usage when it is not the final output (since this is a viewport setting at least I would not expect it to)

Jacques Lucke commented

2024-02-05 16:51:14 +01:00

@SimonThommes Also mentioned that he didn't like that the simplification level affects nodes like this, so maybe the solution should go in a different direction. E.g. the simplify level could be an input node that can be used explicitly if desired. Needs some more discussion.

👍 1

Philipp Oeser commented

2024-02-05 18:12:51 +01:00

I can see how doing simplification might work better when done differently (not sure if an input node overcomplicates this though), however this fix is just about it not updating at all for non-volume objects. So this PR will not do the whole redesign of simplification of volumes (this is not what it intends to do). Do you think it would block the redesign or make it more complicated?

Philipp Oeser referenced this pull request

2024-02-06 09:16:21 +01:00

Mesh to volume node not working in this particular file #117747

Jacques Lucke requested changes 2024-02-06 17:48:16 +01:00

source/blender/makesrna/intern/rna_scene.cc Outdated

						
				@ -2085,1 +2098,3 @@

				  rna_Scene_simplify_update_impl(bmain, sce, false);

				  Scene *scene = (Scene *)ptr->owner_id;

				  Main *bmain = CTX_data_main(C);

				  Depsgraph *depsgraph = CTX_data_ensure_evaluated_depsgraph(C);

Jacques Lucke commented

2024-02-06 17:48:13 +01:00

Ensuring an evaluated depsgraph just to set a property seems a bit too much and could potentially result in double depsgraph evaluation on e.g. render farms that set this property directly after loading the .blend file before evaluation starts.

It's probably ok to just use the result of the previous evaluation, and if that doesn't exist yet, ignore it.

Ensuring an evaluated depsgraph just to set a property seems a bit too much and could potentially result in double depsgraph evaluation on e.g. render farms that set this property directly after loading the .blend file before evaluation starts. It's probably ok to just use the result of the previous evaluation, and if that doesn't exist yet, ignore it.

lichtwerk marked this conversation as resolved

Philipp Oeser added 2 commits 2024-02-07 09:51:27 +01:00

0612d59b03 Merge branch 'main' into 117747_simplify_volume_resolution_update

4a0d8788b5 dont insist on fully evaluated depsgraph

Philipp Oeser requested review from Jacques Lucke 2024-02-07 09:51:55 +01:00

Jacques Lucke approved these changes 2024-02-07 10:07:14 +01:00

Philipp Oeser merged commit b56192b3bb into main

2024-02-07 10:32:15 +01:00

Philipp Oeser referenced this issue from a commit

2024-02-07 10:32:18 +01:00

Fix: Simplify "Volume Resolution" not updating for non volume objects

Philipp Oeser deleted branch 117747_simplify_volume_resolution_update