利用者:Aligorith/Foundation/2018/20180130-GPBranchReviewのソースを表示

= GP Branch - Code Review Points (Aligorith) - 20180130 =

This doc lists some of the problems with the GP branch code that I've identified while trying to fix other issues today. I'm not sure where the best place to put this is to keep this a public doc (i.e. the code-review is too outdated atm, the Trello board is private, blog is probably not best venue), so I figured I'd leave this here until a better idea comes to mind)

-- Aligorith (20180130)


== Big Issue 1 - Modal operators temporary data structs get defined in a public header (ED_gpencil.h) ==
These are *private data* (i.e. internal state of operators) and not part of the *public API* of the module. The fact that we need to share this with the draw manager (i.e. some nasty cross-module coupling) is problematic.

==== Related Issues ====
* ED_numinput.h is included in ED_gpencil.h, because the temp operator structs use NumInput inline. We need a resolution here, as it's not nice having to include a header in a header!

==== Ideal Solution ====
Ideally, we need to completely review what information is needed by each operator at draw time vs while it is being used. Knowing that, we should then only require that only the *minimum* set of data needed by the draw manager callbacks to do their job should be passed to those callbacks. Public headers/function exports should be adjusted accordingly.

(TODO: Check how other modules handle this! I suspect we really shouldn't even be doing any cross-module stuff here)

==== Resolution ====
As a stop-gap measure, at least move the structs out of the public header (into gpencil_intern.h), and only use "struct XYZ" in the parameter lists instead of needing to have the actual structures defined there too.



== Big Issue 2 - Non "2D/3D Art" workflows completely broken ==
Originally I had intended to tackle this after the dust from the branch settled (perhaps as a second branch), since the current branch already has far too many changes in it! However, it's becoming clear that we may have to tackle this in-place sooner rather than later, given that changes in the branch are increasingly going to '''cause issues trying to support *any other* workflow'''.

For example:
# GP Strokes can *only* be drawn in Draw mode, on the *active object* - This completely breaks any annotation workflows and/or addons that were using GP as a way of getting freehand sketch input. The problem is that, since we have to switch the active object and change the mode, the workflow for using those other tools just becomes impossibly clunky. 
** For example, you'd draw one stroke, then have to manually re-select the original object, get back to the right mode, and then maybe reactivate the next tool... or maybe addons automate that changing-back step, but it's still an arguable backwards step! In other words, '''it's impossible to just get the user to draw a single stroke, and operate on it'''
** Due to all the palettes/smoothing/etc. stuff, we can't easily set a colour for a particular operator's strokes. Instead, there's a lot more overhead that needs to happen now.
# 2D editor GP seems to be broken (even in 2.8), though the exact cause is not immediately clear yet. For now, I don't know how screwed-up the current changes have made things for our ability to do stuff in any of the other modes/editors.



== Big Issue 3 - Code Layout (UI Scripts) ==
Currently, the situation is a bit of a mess. The "3d GP Object" stuff has kindof cannibalised the UI code that's supposed to be used for *all* GP UI layouts across different contexts. However, since the 3D GP stuff is being designed for use in a very different way (i.e. it can occupy heaps of separate panels, as it gets its own dedicated tabs/regions/etc. to play in), all other contexts will still need to be much more constrained.   (For example, the "Shapes/Measurement" panel in the 3D view used to be the general "Tools" tab.  Other things like the layers UI also come to mind.)

==== Proposal ====
# Split out all the 3D GP stuff into new files (or simply into the 3d toolbar file, though that too is growing far too big for its own good). 
# Restore the "common" code back to the non-branch state. (TBD, should we remove even the now unused 3D view handling stuff?)


== Big Issue 4 - Render Integration ==
TBH, I'm not too familiar with these issues. From my perspective, this isn't really something that needs to be solved *now* (i.e. pre-merge), unless support for the existing OpenGL render goes away before the branch gets merged into 2.8.


== Code Issues / UI Issues ==
There are various other smaller issues I've found/know about so far (and a handful more)

* GP Monkey doesn't act like any other primitive. It *always* adds a new object (and cannot currently be made to just add points to an existing object, as it can't transform the points)
* Circle/Rectangle primitives are modal tools only. It would be good to get non-modal versions that can be activated from the Add Menu, that will behave more like the other primitives, setting things up with default settings.
* Duplicate code - e.g. <code>copy_v2int_v2int()</code> and friends in <code>gpencil_paint.c</code>
* "no_straight_lines" and "no_fill" properties for the GPNECIL_OT_paint operator should be inverted (booleans should usually define the positive form)


* ... Many many other small things like this if you start digging into it ...

== Mergeability / Planning Issues ==
I'm conflicted about whether to recommend we merge earlier (and then fix the issues slowly), vs only allowing a merge once everything is fixed and works nicely!

Pro Early Merge:
* Throughout most of December, we were wasting a lot of time trying to keep the branch on top of all the 100's of commits per day making heaps of breaking/conflicting changes all over (and particularly in the depsgraph area). The longer we keep things in the branch, the longer that we waste time struggling to keep things in sync (and fixing sudden production-stopping crashes that weren't there an hour or 2 ago). If the code were merged however, such breakages wouldn't be a problem as the problem areas would already have been fixed when such big changes get made.

* By pushing the branch to 2.8, we get a lot more eyes on all the stuff going on here. Including faster detection + fixing of "quirky UI" / "redundant code" / etc.

* arcanist and phab are really starting to struggle and complain everytime I try to update the diff. So far, I've only been able to update it here by manually creating a diff file and uploading it, which seems to break some other tooling that may be available otherwise.

* We can avoid further risks of version numbers getting out of sync (and having trouble correctly patching files). This has happened a few times already, it will happen again. I don't look forward to having to figure out if the new version bump will break more stuff...

* More users can easily test this stuff more regularly


Cons of Early Merge:
* Should we really allow dodgy code and bad paradigms to get merged into the main branch, where there won't likely be further code reviews to identify and fix it, and setting the precedent for "more of the same" to keep happening?  One advantage of the branch right now is that we can diff it against 2.8, and see at a glance all the places where stuff has been changed. So far, some of those have been quite surprising, and really did need to be addressed.


'''Personal Position/Cutoff Line:'''
IMO, as long as there aren't many "major" issues, we should merge it earlier and then slowly fix all the smaller remaining issues. Here "major" refers to 2 main points: 
# Changes in the data structures stored to file - Put simply, if big file structure changes are needed, it's easier to manage the changes for a small crew in a branch, rather than in public for everyone who may have a build
# Structural/API changes that will affect a large number of files all over Blender - We should avoid merging if we foresee the need to make some big changes to saved files or many parts of Blender, causing churn for everyone (users + devs alike), with risk of data loss and nasty merge conflicts