GP Branch Review Meeting (11 May) #55002

New Issue

Joshua Leung · 2018-05-08T12:52:02+02:00

Joshua Leung commented

2018-05-08 12:52:02 +02:00

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 #54721, #54769)
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, @ideasman42 , @JoshuaLeung, and @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.

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

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 #54721, #54769) * 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, @ideasman42 , @JoshuaLeung, and @mont29 sat down and went through and did a lengthy review of the patch ([D2889](https://archive.blender.org/developer/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. @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** - @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).

Joshua Leung self-assigned this 2018-05-08 12:52:02 +02:00

Joshua Leung commented

2018-05-08 12:52:02 +02:00

Added subscribers: @JoshuaLeung, @antoniov, @ideasman42, @mont29, @dfelinto

Joshua Leung commented

2018-05-08 14:22:56 +02:00

Added subscriber: @WilliamReynish

Antonio Vazquez commented

2018-05-08 15:01:19 +02:00

@JoshuaLeung Do you want I start the task to change of Modifier struct names?

Antonio Vazquez commented

2018-05-10 16:33:39 +02:00

Added subscriber: @pepe-school-land

Antonio Vazquez commented

2018-05-10 16:33:39 +02:00

I had a meeting with @pepe-school-land 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 @JoshuaLeung suggested.

I had a meeting with @pepe-school-land 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 @JoshuaLeung suggested.

Antonio Vazquez commented

2018-05-10 18:12:59 +02:00

@JoshuaLeung I have finish the rename of the modifier structs: 43993d7a55c8 d0946b84209d

Antonio Vazquez commented

2018-05-11 10:24:09 +02:00

@JoshuaLeung any update with the meeting decisions?

Joshua Leung changed title from ~~GP Branch Review Meeting (9 May)~~ to GP Branch Review Meeting (11 May)

2018-05-11 12:27:22 +02:00

Joshua Leung commented

2018-05-11 16:18:00 +02:00

Added subscriber: @fclem

Joshua Leung commented

2018-05-11 16:18:00 +02:00

This afternoon, @ideasman42 , @JoshuaLeung, and @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.

@fclem also performed a review of the drawing code (comments pending)

This afternoon, @ideasman42 , @JoshuaLeung, and @mont29 sat down and went through and did a lengthy review of the patch ([D2889](https://archive.blender.org/developer/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. @fclem also performed a review of the drawing code (comments pending)

Dalai Felinto commented

2018-05-14 10:21:25 +02:00

Changed status from 'Open' to: 'Resolved'

Dalai Felinto closed this issue

2018-05-14 10:21:25 +02:00

Daniel Martinez Lara commented

2018-05-21 14:42:08 +02:00

@JoshuaLeung 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

@JoshuaLeung 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

Joshua Leung commented

2018-05-22 17:52:12 +02:00

@pepe-school-land 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

Antonio Vazquez commented

2018-05-22 17:56:02 +02:00

@JoshuaLeung You mean create an operator to convert the 2.7 files and don't use the versioning code?

Joshua Leung commented

2018-05-22 17:59:38 +02:00

Yes

Antonio Vazquez commented

2018-05-22 18:27:28 +02:00

@JoshuaLeung Ok, I will try to create this operator.

Do you have any favorite name? "GPENCIL_OT_convert_old_files"?

@JoshuaLeung Ok, I will try to create this operator. Do you have any favorite name? **"GPENCIL_OT_convert_old_files"**?

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

GP Branch Review Meeting (11 May) #55002

Key topics for review

Outcomes of Meeting