[Bf-committers] DNA structs with local scope

[Sergey Sharybin]

Hi.

Ok, now i see the issue you're trying to solve. However, the solution does
not seem to be right at all to me. Here's a summary of concerns.

1. In order to make your own module more sane you're completely tearing
apart another module. It is REALLY important to keep all DNA in a single
place in order to keep changes in there more consistent and atomic. If the
header files are scattered across the whole Blender source code you're
asking for really big problems to happen: you wouldn't be able to easily
grep all usages of ID_NAME and bump it up, you wouldn't be able to make
sure some non-portable types are used and so on.
I would REALLY suggest everyone to be careful here. We had several
occasions when coder mistake in DNA files caused .blend file corruption. I
see why others will be worrying about minimizing effort and mistake-ability
in other modules, but NOT in the cost of sanity of other modules.

2. Your solution is completely binary: you either hide the whole structure
or nothing. It is probably fine for a small structures, thing of the future
and other datablocks. With your approach you'll require writing
getters/setters for EVERY structure member which is a complete overkill.
Points here are:

  - You should not have getter/setter for a simple accessing properties.
For example, you should not have getter like: int BKE_pchan_get_flags(bChan
*pchan) { return pchan->flag; }; If you require doing this then extending
structure becomes really a pain: you'll need to modify like 4 different
files before you can use new property.

  - We are in C world, where we don't have polymorphism. Let's get an
example of Object->obmat. You want to access that for both const and
non-const Object pointers. For a const object you'll want matrix to be also
const (and you NEVER should do const_cast!). So you'll need to have two
getters:

    float (BKE_object_get_matrix(Object *object))[4][4];
    const float (BKE_object_get_matrix_const(const Object *object))[4][4];

Having such a _const suffix is becoming really verbose and annoying.

  - Performance will become quite a concern. Don't think those getter
functions can be easily inlined (unless you define them as BLI_INLINE in a
header file, causing the WHOLE Blender to recompile next time you change
logic in some of the getters). And you wouldn't want per-vertex function
call to get vertex coordinate when dealing with mesh operations.

3. Writing RNA for such structures will become really crazy. PROBABLY, RNA
will pick SDNA up nicely, but at the end of a day it means that RNA is
allowed to modify ANY property directly. Also, if you'll need to have any
poll-like or update-like function you'll either end up in a nest of
accessor calls or you'll end up including "private" header

4. Versioning will become a pain as well. You'll either pierce the
"private" header to the do_version files (which is defeating original idea
of clear separation and such) or you'll end up in lots of legacy accessors
which are only supposed to be used for versioning purposes (and people will
probably will try to use them for some real work just because they are in
place).

5. When/if some existing DNA will want to be switched to this new idea, it
means it'll start scattering more and more from DNA folder.

I can probably keep going into some more specific cases, but those are
bothering me and i do not see the suggested approach an appropriate
solution. For until there's some real smart approach is found which will
solve the issue without introducing others i would REALLY suggest sticking
to the current design and follow:

- All DNA files are stored in makesdna/
- Use comments about how you want something to be accessed (i.e, "DO NOT
include this file directly in user-space tools, use BKE_foo.ih instead" or
"DO NOT access this field directly, use BKE_foo_get_some_magic()."). Doing
documentation is ALWAYS a good idea, so not asking for anything new here.
- Review changes in your area and point developers to a wrong usage of the
things. Once again, this is something you are supposed to do anyway, so
nothing new either.


P.S. Can think of a good recent example when issue which is really close to
your current one was solved in a really clear way which did not violate
anything: the ID_IS_LINKED_DATABLOCK() commit.


On Sat, Dec 10, 2016 at 6:03 PM, Julian Eisel <eiseljulian at gmail.com> wrote:

> Hi,
>
> let me try to make my motivation a bit clearer, long-ish read ahead :)
>
> I'd say this is really about code architecture. Simply put I want to
> improve modularity and force people to use abstractions/APIs rather
> than manipulating data directly. We barely use any abstractions for
> DNA struct members, we simply access them all over the place (since we
> can include their header file directly).
>
> This summer I spent a lot of time with going through the code base to
> change all low-level accesses of layer bits-fields. This would've been
> a matter of minutes if there were API functions, like
> BKE_object_layer_is_visible(...). Now, I'm in a similar situation with
> integrating workspaces into the existing design and there have been
> many similar situations before.
> And of course, API usage can greatly improve readability
> (BLI_object_layer_is_visible(...) vs. (v3d->lay & ob->lay) != 0). I'm
> really passionate when it comes to code readability and can't stress
> its importance enough.
>
> Let me use custom-manipulators as another example: There are structs
> for managing the individual manipulators, their states, their
> updating, etc, *and I don't want any code outside of the wmManipulator
> module to directly access or even change their data!* Right now, they
> are local structs and other parts of the code can request information
> from them *through an API*. It's however well possible that I'll have
> to move these structs into DNA to support file read/write of
> manipulator states. Right now that would mean moving them into a
> global DNA file, allowing people to bypass the API and manipulating
> data directly.
> We can of course simply ask developers to use APIs/abstractions, but
> in the code I maintain I would really prefer to force doing this. With
> the current global DNA headers I won't be able to do so though.
>
> Another thing: On one hand we're really strict about not calling any
> WM functions from BKE, on the other hand we can simply include
> DNA_windowmanager_types.h in BKE and access the WM structs directly
> (and we actually do this). IMHO the only code that should know how the
> wmWindow struct looks like, is the WM code itself, or even only
> wm_window.c. If any other part of the code wants to manipulate data of
> a wmWindow instance, it should do so through an API. (This is just an
> example, I don't propose to change this now.)
>
> I agree with what Hermann said, all this is probably a consequence
> from the ancient global shared data design of Blender. What I want to
> do is to move away from global data and move towards a more modular,
> API based design to make maintenance and refactoring easier - at least
> in the code modules I work in and maintain. Hence my proposal to allow
> local DNA headers.
>
> Cheers,
> - Julian -
>
> On 9 December 2016 at 20:07, Ichthyostega <prg at ichthyostega.de> wrote:
> > On 09.12.2016 18:04, Julian Eisel wrote:
> >> I wanted to discuss a thing about DNA that I really don't like: DNA
> structs
> >> are always placed in headers that we allow to access globally within the
> >> source code.
> >
> > to me this looks like a direct consequence of Blender being built round
> > a common global shared data model. And I agree with you, today you would
> > think twice before starting a new *large scale* application with that
> > kind of architecture. But that is another story.
> >
> > OTOH, I would also be reluctant to hide the underlying architecture,
> > with some kind of markup or metadata. Let's just face it: in Blender,
> > every part is a tool that works on a common model, and it's up to
> > the developers to use that power wisely.
> >
> > Cheers,
> > -- Hermann
> >
> >
> >
> >
> > _______________________________________________
> > Bf-committers mailing list
> > Bf-committers at blender.org
> > https://lists.blender.org/mailman/listinfo/bf-committers
> _______________________________________________
> Bf-committers mailing list
> Bf-committers at blender.org
> https://lists.blender.org/mailman/listinfo/bf-committers
>



-- 
With best regards, Sergey Sharybin