2023-07-25 Sculpt/Texture/Paint Module Meeting

Meetings
1

Attendees

  • Dalai Felinto
  • Julien Kaspar
  • Sergey Sharybin
  • Daniel Bystedt
  • Hans Goudey

Announcements

  • 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 available to work on the module.
    With recent developments this might take longer because wrapping up current sculpting tasks likely will take precedent.

  • Joe Eagar will no longer be employed by the end of the month.
    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.

Agenda

High Priority Bugs

Current state of the module and short term plan

The module remains for now in an orphaned state without a module owner. Module meetings will be halted until more active development continues.

  • Julien will coordinate and keep an eye on the state of the module just like before.
  • Sergey will be available for important bug fixes.
  • Hans is also interested in branching out into the module and tackle technical topics, performance issues and fixing technical debt in the code.
    This would help fix various bugs and open doors for new features.
    His area of experience is more for mesh and sculpting, so he likely won’t be available for textures and udim work.

Existing developers are already involved in many project and can’t immediately help out in module projects. Until new developers join the module the focus will be on Julien & Daniel to define the strategic goals of the module:

  • Wrapping up existing patches (Dyntopo and Roll Brush)
  • Design goals and requirements of Multires and Layered Sculpting (Sergey will eventually be able to work on this)
  • Any vital features and fixes to strengthen the experience of sculpting in Blender

Daniel & Julien will also actively contribute to the Brush Assets project to make the Brush Essential Assets ready for sculpting and painting modes.
Julian Eisel and Bastien Montagne are also involved in that project for porting the tools over to brush assets in the code.

Aligning resources and developer time to finish the Dyntopo PR

A lot of time and resources were invested in reworking Dyntopo and the feature is in a relatively final state.
Although the rest of the team is busy now, there would be time soon to tackle this as an internal project (if it is still waiting then).
Sergey will still take a few weeks to wrap up compositor work and Hans is taking time off soon.

Dalai asked Sergey to analyze how he would suggest the patch to be split (see below in this document).
This is intended to map out what the initial future step would be for the development team.
But it can also serve as a guideline in case Joe wants to keep iterating on the patch (given that he did a few updates on it since last week).

Split Strategy for Dyntopo

Summary

General notes

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:

Splittable parts

Based on a quick pass over the patch and personal experience of structuring projects.

NotForPR

Changes marked as NotForPR should be removed prior to asking for a review.

startup.blend

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.

MSVC + Clang changes

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.

BKE_dyntopo_set.hh

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:

  • Make it a BLI primitive
  • Cover it with regression tests
  • Include (rudimentary) benchmarking, similar to the BLI_set_test.cc

BLI_heap_minmax.hh

Move to a separate PR and consider:

  • Expand the description. I.e. it states it does priority queue, so how is it comparable/different from std::priority_queue.
  • Cover with regression tests

BMesh edge collapse

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.

Edit BMesh fair vertices

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.

CD_FLAG_TEMPORARY

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

ATTR_NO_OPT

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.

BLI_mempool.h

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.

BMesh tool flags

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.

geometry_attributes.cc / mesh_data.cc

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.

join_mesh_single

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.

space_image folder

Seems to be a lot of changes, and not needed for the dynamic topology project.

Auto-save changes

Needs to be split out, and seems to be already submitted as #110088
The issue is the performance and UX.

Closing notes

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.

23 Likes
2

Thank you. Hope you find quickly a new dev to replace Joe

2 Likes