[Bf-committers] DNA structs with local scope

[Bastien Montagne]

Hey,

I fully agree with Sergey here.

I understand the concern, but, besides all technical reasons listed in 
above mail (could also add the whole read/write code, which absolutely 
needs raw access to whole DNA, since it's a mere binary dump of those 
structs), I fully adhere to the (pythonic) mantra: Trust the developer, 
do not try to forbid him/her from doing something stupid - people will 
always do stupid things if they are up to it, anyway.

So yes, comments and the like are more than welcome to document how to 
use/access some data. But please, no enforced 'private' DNA data with 
'public' accessors. Having freedom does not mean that you should abuse 
it, but potential abuses of freedom should not be a reason to limit it 
either.

Bastien

Le 11/12/2016 à 11:27, Sergey Sharybin a écrit :
> 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
>>
>
>