Task to keep track of results from GP branch review meeting #2 (scheduled for
9 May 11 May, in Blender Code Quest HQ)
Key topics for review
- Materials changes - are all of these things working soundly (lib handling, file IO, usercount, indexing, etc.)?
- Mode toggle operators (GPENCIL_OT_editmode_toggle, GPENCIL_OT_sculptmode_toggle, GPENCIL_OT_weightmode_toggle, GPENCIL_OT_paintmode_toggle)
- Should these even be separate operators?
"back" parameter- (Standard Blender behaviour, though wonky)
- Hotkeys (e.g. can currently get "locked" in editing modes, with no way to return to object mode) - (Not urgent for merge, but needs review)
- It was agreed earlier that we don't need strict compatibility with Hero files. With some (VFX) modifiers removed, should we consider fixing the naming of structs/identifiers for GP modifiers too (e.g. currently they're all eModifierType_Gpencil<ModName> and Gpencil<ModName>ModifierData, which may be nicer as eModifierType_GPencil_<ModName> and <ModName>GPencilModifierData instead? It'd be great to not be stuck with the current naming for eternity.
- "GP Paper" setting in 3d viewport -
Should this become an overlay type, if so, where to put its options. (Also is this for before/after merge?)
- Discussed this in office before meeting: Probably should become an overlay, and the settings can go into the overlay popover like the Editmode settings currently do. But, we can leave that till after the merge, to get things in faster.
- Are there any other blocking issues for merge?
Outcomes of Meeting
This afternoon, @Campbell Barton (campbellbarton) , @Joshua Leung (aligorith), and @Bastien Montagne (mont29) sat down and went through and did a lengthy review of the patch (D2889), starting with DNA, then moving on to RNA, and some of the blenkernel changes.
We identified a number of issues (specifically in the DNA structures), that need to be resolved - mainly naming issues with structs/variables, and runtime data needing to get moved out to nested structs instead of polluting the general datablocks. These have all been noted in the patch (https://developer.blender.org/D2889#77739). Given that many of these were problems in the DNA code, changes will need to be made to all downstream places using these.
Note: Certain issues were excluded from review (as things that can be tackled later, post-merge, given the complexity of the issues there). Specifically:
- GP's own COW system (i.e. the derived_gpf, etc. GHashes) - The work involved in properly integrating this with Blender's proper depsgraph/evaluation/cow systems is complex, and would require significant effort/time investment away from other code quest targets. Current conclusion is to just leave this as-is, with the proviso that this is "Bad Code (TM)" that needs to be cleaned up in the long run.
- UI Layouts and .py code for those - These will need to be reviewed and redone with the UI team anyway (once the toolbar work stabilises), so we will leave those reviews for a post-merge review pass.
- Blenkernel / ID /Library Management Stuff - @Bastien Montagne (mont29) was going to do a separate, careful review of these, as this needs careful study.
- Operator changes. There were enough other changes needed, that we did not cover these today. To be left for a future review.
Blocking Issues for Merge:
Resolution of all issues identified so far (in comments on the patch).