Page MenuHome

GP Branch Review Meeting (11 May)
Closed, ResolvedPublic

Description

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.)?
  • Check up on other issues raised during last review (see T54721, T54769)
  • 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.

@Clément Foucault (fclem) also performed a review of the drawing code (https://developer.blender.org/D2889#77859)

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).

Details

Type
To Do

Event Timeline

Joshua Leung (aligorith) triaged this task as Normal priority.

@Joshua Leung (aligorith) Do you want I start the task to change of Modifier struct names?

I had a meeting with @Daniel Lara (Pepeland) (pepeland) and we agree to make the rename of the struct for modifiers.

After the rename, we will recreate in the new version the modifiers manually or using a script to export or patch old files.

I'm going to start the rename task of structs as @Joshua Leung (aligorith) suggested.

Joshua Leung (aligorith) updated the task description. (Show Details)
Joshua Leung (aligorith) renamed this task from GP Branch Review Meeting (9 May) to GP Branch Review Meeting (11 May).

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.

@Clément Foucault (fclem) also performed a review of the drawing code (comments pending)

Dalai Felinto (dfelinto) closed this task as Resolved.May 14 2018, 10:21 AM

@Joshua Leung (aligorith) is it possible enable again the possibility to open the 2.79 GP files in the 2.8 GP branch? we have the Anncey demos in the Dlender booth the next week and when I open any GP file from 2.79 do not appear the GP data.

Daniel

@Daniel Lara (Pepeland) (pepeland) I suggest looking for the operator to convert old data/scenes to the new GP objects, and then making "2.8 ready" versions of the files

@Joshua Leung (aligorith) You mean create an operator to convert the 2.7 files and don't use the versioning code?

@Joshua Leung (aligorith) Ok, I will try to create this operator.

Do you have any favorite name? "GPENCIL_OT_convert_old_files"?