WIP: Refactor: Remove extra node mark calls #123760

Sean Kim · 2024-06-26T01:26:53+02:00

Sean Kim commented

2024-06-26 01:26:53 +02:00

These calls should be redundant based on the overall handling in
push_undo_nodes.

Still need to do testing, but from a look at the code I don't think these are needed.

These calls should be redundant based on the overall handling in push_undo_nodes. --- Still need to do testing, but from a look at the code I don't think these are needed.

Sean Kim added 1 commit 2024-06-26 01:27:04 +02:00

Refactor: Remove extra node mark calls 3a0911a91c

These calls should be redundant based on the overall handling in
push_undo_nodes.

Sean Kim added this to the Sculpt, Paint & Texture project 2024-06-26 01:27:08 +02:00

Hans Goudey reviewed 2024-06-26 02:25:27 +02:00

Hans Goudey left a comment

Thanks for the cleanup. However I think I'd rather mark nodes as changed in the brushes, so removing the update tags from the undo pushing loop. Reasoning:

Same as the general transition to putting more code in each brush implementation-- reducing implicit coupling of different areas.
If we want, we can more easily detect if the brush actually changed anything before tagging updates.
We could more easily eagerly recalculate some caches like the bounds cache without affecting the node's tag.
Reducing the amount of time where the tag is "incorrect". i.e. put the tag right next to the thing that actually changes the data.

Thanks for the cleanup. However I think I'd rather mark nodes as changed in the brushes, so removing the update tags from the undo pushing loop. Reasoning: - Same as the general transition to putting more code in each brush implementation-- reducing implicit coupling of different areas. - If we want, we can more easily detect if the brush actually changed anything before tagging updates. - We could more easily eagerly recalculate some caches like the bounds cache without affecting the node's tag. - Reducing the amount of time where the tag is "incorrect". i.e. put the tag right next to the thing that actually changes the data.

Sean Kim commented

2024-06-26 02:29:00 +02:00

Makes sense, wasn't sure which direction we were going here in terms of abstraction; I suppose the larger idea for push_undo_nodes is to also move the undo steps along with the node marking, right?

Makes sense, wasn't sure which direction we were going here in terms of abstraction; I suppose the larger idea for `push_undo_nodes` is to also move the undo steps along with the node marking, right?

Hans Goudey commented

2024-06-26 02:39:04 +02:00

Yeah that was my thought. Though I'm a bit less sure of that after recent changes significantly decreased the undo system's overhead. We'd probably want to test if pushing undo data inside the brush evaluation node loop improved performance or not. On top of that there might be some opportunities for synergies between the two. Theoretically for nodes that need to gather(positions_eval, verts, positions) we could skip doing that another time when storing undo step data.

Yeah that was my thought. Though I'm a bit less sure of that after recent changes significantly decreased the undo system's overhead. We'd probably want to test if pushing undo data inside the brush evaluation node loop improved performance or not. On top of that there might be some opportunities for synergies between the two. Theoretically for nodes that need to `gather(positions_eval, verts, positions)` we could skip doing that another time when storing undo step data.

Sean Kim commented

2024-06-27 01:24:58 +02:00

Closing this, since we aren't moving in this direction for the overall brush refactor.

Sean Kim closed this pull request

2024-06-27 01:25:00 +02:00

Sean Kim deleted branch refactor-remove-positions-update

2024-07-24 02:11:28 +02:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

WIP: Refactor: Remove extra node mark calls #123760

Pull request closed