Note: If you're planning to join the meeting, feel free to add talking points to the agenda.
2023-07-13 Texture Painting Polishing Workshop
A small workshop to discuss possible targets for improving the texture painting experience.
No definitive goals were set yet. The meeting notes will be used to align short term targets with core developers becoming availible to work on the module.
Joe Eagar's contract ended.
Ton's statement:
For personal reasons, Joe Eagar, one of the Sculpt/Paint module team members, was asked to step aside. His development grant (which lasted 18 months) will be ended by the end of July as well.
Julien will go over repeat setting and view plane bug.
Hans can look into "Dyntopo Smooth Shading Bug"
Discusssing the current state of the module and how to continue in the short term.
Remaining development targets for 4.0 including high priority fixes.
Aligning resources and developer time to finish the Dyntopo PR.
The presentation of the project needs to be updated.
The project is not about refactoring, but about bringing new functionality: performance and attribute preservation. Refactoring by its definition does not change an external behavior.
The description needs to be updated to become a single-read self-sufficient information. Start with the description of the problem the patch solves, description how it is done.
Referring to state of other branches and their history should be avoided in the general part of the PR description. It is fine to have such information later on (after the "–-" marker). It is important to give context about such referring, because reviewers are not familiar with the state and history of code outside of the PR.
Inspirational read:
Based on a quick pass over the patch and personal experience of structuring projects.
Changes marked as NotForPR
should be removed prior to asking for a review.
It is hard to tell what exactly is changed in the file.
A lot of versioning is to be done in the versioning_default.cc
. There might be a good reason to update the file, but more information about it is needed.
Similar to other platforms and compiler configuration we are open for compilation error fixes. That being said, the compilation fix needs to be submitted and presented separately.
There are likely other places where iteration over unique set of elements is needed, and the implementation does not seem to be bound to Dynamic Topology. Should consider:
Move to a separate PR and consider:
The current PR description states that the current algorithm in the main branch has bugs. Having a more robust edge collapse is a general good improvement to have outside of dyntopo. It should be presented and reviewed from this pesrpective.
The implementation probably needs a re-iteration, as it is a bit strange to generate specializations of such non-trivial algorithm for all types of callbacks.
It does not seem to be used in the patch, and the intent is not really clear. If this is a new operator which is intended to be exposed it should go into a separate PR.
Such semantic change should be isolated to own incremental step. This is likely to touch some API changes in the BKE_attribute.h and BKE_customdata.h
Either needs to be completely removed, or presented as a separate PR.
It was mentioned the change was rejected, but I did not find a PR to get more context on this topic.
From personal experience sometimes it is handy to be able to disable optimisation of a specific function during development/troubleshooting. Being able to do so without looking up for an exact syntax sounds good. So to me it does seem to be a reasonable thing to have.
The patch introduces BLI_mempool_get_size
. Is not very clear what the difference from BLI_mempool_len
is. If it does something else it needs to go to a separate review.
The belief is that it is possible to extract this part of the patch, keeping it no-functional-changes for the current state, and review it from this perspective.
Doing so will give ability to present the need of such change and understand it and validate more efficiently, as well as removing bulk of changes related on the API change.
From reading the code it seems that it is actually a bug fix to something that is already in the main branch: make undo in sculpt mode do what one expect it to do after adding attribute.
If it is indeed so, the change needs to be split and explained what the fix mean on user level.
It seems that this is a change which Blender benefits from outside of the dynamic topology project. So seems reasonable to split the change out and review it by the modelling team.
Seems to be a lot of changes, and not needed for the dynamic topology project.
Needs to be split out, and seems to be already submitted as #110088
The issue is the performance and UX.
Surely the notes above do not cover the juicy parts of the actual dynamic topology changes. The goal so far was to bring the patch in an easier to review state. With the size of this patch it takes a lot of mental energy to even go over those easy-to-separate changes, and it will be an iterative process to tackle the entirety of the patch.