We only updated the mesh attribute request to undisplaced position
when switching from "Displacement and Bump" to "Bump Only", but
we forgot to do it when switching from "Displacement Only" to
"Bump Only"
Details
Details
- Reviewers
Kévin Dietrich (kevindietrich) Brecht Van Lommel (brecht) - Maniphest Tasks
- T75539: Cycles: switching from Displacement to Bump Only does not update geometry
- Commits
- rCa7d2f9187d9d: Fix T75539: Cycles missing geometry update when switching displacement method
rB68d5ad998393: Fix T75539: Cycles missing geometry update when switching displacement method
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- T75539 (branched from master)
- Build Status
Buildable 10203 Build 10203: arc lint + arc unit
Event Timeline
Comment Actions
Undisplaced positions are only needed for "Displacement and Bump", and always enabling them for "Displacement" wastes memory.
Adding them here will indirectly force the geometry to be updated, but we need a more direct solution. Probably the geometry could store if it has been displaced, and then if displacement method is bump and there is displaced geometry, it can be marked for update.
Comment Actions
Did you mean something like this? I am getting familiar with the code, so I hope I am not that off
Comment Actions
Hi @Brecht Van Lommel (brecht), as adviced on the blender-coders chat, I added "Render & Cycles" as reviewers too, given that I have seen in a recent meeting minutes that you asked for other's help on reviews.
Comment Actions
It's individual geometry that should be marked as being displaced, not the geometry manager for all geometry.
I don't immediately have advice for how to best implement that, it may be a more complicated change.
Comment Actions
What about adding a bool member to Mesh class "is_displaced", and then set it to TRUE for any mesh that is displaced in GeometryManager::displace() (in mesh_displace.cpp). I may be wrong but it looks to me that by doing that at least we would cover the first part: having individual geometry (at least meshes) marked as being displaced. If that works, what if we then set
geometry_manager->need_update = true
if we find a Mesh in the scene with "is_displaced == true"? would that make sense? Perhaps is actually "Scene" class that can have a boolean too if any of the meshes in it was displaced and then mark it for an need_update as I did.
Comment Actions
Yes, is_displaced should be a member of Geometry.
Probably the solution involves tagging shaders as having their displacement property changed, and then in the geometry update looping over the meshes and tagging them for updated based on the shader updates.
Comment Actions
Brecht I know you originally mentioned that the fix should not land in Shader::tag_update but Kévin explained to me that after his change landed, the actual check of whether displacement method was modified is pretty simple. The actual fix proves that idea.
Comment Actions
This looks good, however the geometry_manager should also be tagged for an update. For now this works only because the geometry manager is tagged indirectly for an update in the Blender exporter; we don't want to depend on such behavior if it ever changes, which it might in the future with more granular updates (D9555 and beyond). The object_manager should also be tagged for a flags update (need_flags_update) since the change in displacement affects the geometries' bounds and we would then need to update the possible intersections with volumes.
| intern/cycles/render/shader.cpp | ||
|---|---|---|
| 329 | Should be: if (has_displacement && displacement_method_is_modified()) You should also move this check below the update of the has_displacement flag below, and perhaps merge this with the other check on displacement_method on line 354, e.g.: if (has_displacement) {
if (displacement_method == DISPLACE_BOTH)) {
...
}
if (displacement_method_is_modified()) {
...
}
} | |
Comment Actions
@Kévin Dietrich (kevindietrich) I'll assume you're reviewing this patch now, unless you want me to have a look as well.