Details

Reviewers

Commits

rBbab47b60cb69: DNA: Add space clip editor defaults

Summary

This is my attempt of adding defaults for the space clip editor struct (in line with https://developer.blender.org/T80164)

It adds the default allocation for SpaceClip and node_composite_movieclip.cc.
This also solves the error below (for cpp files using the DNA_default_alloc), which was put forward by @Sergey Sharybin (sergey).

error: invalid conversion from ‘const void*’ to ‘const char*’ [-fpermissive]
   49 |       DNA_default_table[SDNA_TYPE_FROM_STRUCT(struct_name)], sizeof(struct_name), __func__)
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
      |                                                           |
      |                                                           const void*
blender/source/blender/nodes/composite/nodes/node_composite_movieclip.cc:48:25: note: in expansion of macro ‘DNA_struct_default_alloc’
   48 |   MovieClipUser *user = DNA_struct_default_alloc(MovieClipUser);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~
blender/source/blender/makesdna/DNA_defaults.h:39:50: note:   initializing argument 1 of ‘char* _DNA_struct_default_alloc_impl(const char*, size_t, const char*)’
   39 | char *_DNA_struct_default_alloc_impl(const char *data_src, size_t size, const char *alloc_str);
      |                                      ~~~~~~~~~~~~^~~~~~~~

Diff Detail

Repository

rB Blender

Branch

dna_defaults_space_clip (branched from master)

Build Status

Buildable 18969
Build 18969: arc lint + arc unit

Event Timeline

Simon Lenz (Amudtogal) requested review of this revision.Nov 25 2021, 10:51 AM

Simon Lenz (Amudtogal) created this revision.

Harbormaster completed remote builds in B18964: Diff 45269.Nov 25 2021, 10:51 AM

I finished the basic testing of the new default initialisation.

There is one problem/limitation: changes of these defaults will not propagate into the Blender default view configurations (namely VFX).

DNA_defaults: fix default allocation for C++ (sergey)
Add right flag for render size

Harbormaster completed remote builds in B18969: Diff 45276.Nov 25 2021, 2:02 PM

Simon Lenz (Amudtogal) edited the summary of this revision. (Show Details)Nov 25 2021, 2:31 PM

Simon Lenz (Amudtogal) edited the summary of this revision. (Show Details)

Simon Lenz (Amudtogal) added a subscriber: Sergey Sharybin (sergey).

Simon Lenz (Amudtogal) added a reviewer: Julian Eisel (Severin).Dec 1 2021, 12:07 PM

Mostly fine, just suggesting some smaller changes. There are also two issues/bugs, but these should be easy to resolve.

source/blender/makesdna/DNA_defaults.h
49	Just committed rBfd224048375b, so this cast will have to be updated to be `const uint8_t`.
source/blender/makesdna/DNA_movieclip_defaults.h
48	`MoveClipUser` is instantiated in a bunch of places, which won't use the DNA defaults with this patch applied. But they also set up different values after instantiation than what's defined in DNA here. E.g.: `seq_render_movieclip_strip()` `proxy_thread_next_frame()` multiple functions in `clip_editor.c` ... @Campbell Barton (campbellbarton) how should we handle such cases? Should all instantiations use the same defaults from DNA and override them as needed, or is it fine to let them have custom initialization? To me it's fine.
52–53	Defined at the end of `DNA_movieclip_types.h`. E.g. search for `MCLIP_PROXY_RENDER_UNDISTORT`.
69	Where do these magic numbers come from? :) I don't see initialization of these in current code.
72–80	Can be just: #define _DNA_DEFAULT_MovieTrackingMarker \ { \ 0, \ } Other members are guaranteed to be 0-initialized then by the C standard.
source/blender/makesdna/DNA_space_defaults.h
42–43	Make sure the GDB readout is from creating a new editor, so a Clip Editor in an area where there has never been a Clip Editor open before. Otherwise you're not gonna get the newly initialized values, but whatever is set in the existing/restored editor instance. For example in the default Motion Tracking workspace, the zoom level is at `0.793701` which doesn't match the default for new editors, as you've noticed here.
59–60	You can use `_DNA_DEFAULT_UNIT_M4` for these.
source/blender/nodes/composite/nodes/node_composite_movieclip.cc
48–53	Old code doesn't set `MCLIP_PROXY_RENDER_SIZE_FULL` here, with your changes it will be set.

Julian Eisel (Severin) requested changes to this revision.Dec 1 2021, 12:45 PM

This revision now requires changes to proceed.Dec 1 2021, 12:45 PM

Simon Lenz (Amudtogal) added inline comments.Dec 2 2021, 10:13 AM

source/blender/makesdna/DNA_movieclip_defaults.h
48	I think, it would be better to use the DNA defaults everywhere, because extensions of the struct will be much easier to handle that way. Say, adding a new float value to MovieClipUser, if DNA default allocation is used everywhere, the new default value will automatically propagate to all using functions. As a site note: I was trying to find all occurences of MovieClipUser initialisation, but didn't find the functions that you mentioned (using a grep search for the struct). How would you navigate/search the code for this? I will commit the changes soon.

Adjustments after revision

Fixes for revision
Add default allocations for MovieClipUser

Harbormaster completed remote builds in B19148: Diff 45567.Dec 2 2021, 11:40 AM

There we go...

source/blender/makesdna/DNA_movieclip_defaults.h
48	I searched for all occurences of these instantiations: `MovieClipUser = alloc` `MovieClipUser = {0}` `MovieClipUser = memset` The added allocations are all in a single commit. Let me know, if I should revert them.
52–53	Initialised as zero, which is not mentioned in the enum (which is why I got confused). Should we add a `MCLIP_PROXY_RENDER_UNSET` (I guess there is no real use case here)?
69	Indeed, this should be 0 instead!
72–80	Fair. I was following `Explicit is better than implicit.` 😄 . But this is much cleaner, agreed!
source/blender/nodes/composite/nodes/node_composite_movieclip.cc
48–53	With default initialisation to 0, it was also set in the old code (though not explicitly). I double checked with a clean GDB readout, as you suggested. From a node perspective full size seems reasonable as well, or shall I set it to something else?

Julian Eisel (Severin) requested changes to this revision.Dec 2 2021, 3:40 PM

Julian Eisel (Severin) added inline comments.

source/blender/blenkernel/intern/mask.c
1316 ↗	(On Diff #45567)	This leaks memory, same for all the other cases that you've changed to use `DNA_struct_default_alloc()` just now. If we decide to use the DNA defaults here, you can do that like this: MovieClipUser user; memcpy(&user, DNA_struct_default_get(MovieClipUser), sizeof(user)); Or even easier (existing code doesn't happen to do that, don't see a reason why not to): MovieClipUser user = *DNA_struct_default_get(MovieClipUser); Prefer the latter since it's more readable and the compiler will optimize copies of small types with this.
source/blender/makesdna/DNA_movieclip_defaults.h
48	Just look for `MovieClipUser`. There will be a whole bunch of false positives but it's not that bad and it's easy to see which matches are instantiations.
52–53	No strong opinion here, we usually don't do it. A flag value of 0 simply means that no flag is set, no need to define that. You also don't have a define for every possible combination of flags. A bitflag enum is simply to be used differently than a regular enum.
57	Should be 0/false by default, until `BKE_movieclip_update_scopes()` runs.
64–66	These 3 should be 0-initialized and only be set by `BKE_movieclip_update_scopes()` (which runs before drawing).
source/blender/makesdna/DNA_space_defaults.h
33	Should be 0.
56	This is 0 by default currently I think.
58–59	Currently these are initialized to 0, but set before drawing. From what I can see initializing them to the unit matrix instead shouldn't affect anything so that's fine, just noting it here.

This revision now requires changes to proceed.Dec 2 2021, 3:40 PM

Revision 2

Fix memory leak of allocations
Adjust defaults

Harbormaster completed remote builds in B19178: Diff 45607.Dec 3 2021, 11:18 AM

Simon Lenz (Amudtogal) added a comment.Dec 3 2021, 11:18 AM

This comment was removed by Simon Lenz (Amudtogal).

Is there any gain from defining defaults for MovieClipUser?

While it could be argued that it's correct to do so, it's a small struct - currently it's fine to initialize as zero everywhere.

I think the main benefits are consistency (for initialisation purposes) and extendability (growing the struct with some new default value should be easier that way).

However, I do not have a strong opinion on the defaults for MovieClipUser.
I only included it as part of the SpaceClip struct.

Simon Lenz (Amudtogal) mentioned this in D13284: Add mask blending factor to mask editor.Dec 7 2021, 10:56 AM

I have no strong opinion regarding MovieClipUser defaults. Maybe @Sergey Sharybin (sergey) has.

Noted a remaining issue inline.

source/blender/blenkernel/intern/mask.c
1316 ↗	(On Diff #45567)	This still leaks memory. `DNA_struct_default_alloc()` allocates heap memory that is never freed. Use `DNA_struct_default_get()` which just gives you the static memory of the default instance.

This revision now requires changes to proceed.Jan 11 2022, 7:01 PM

MovieClipUser is kind of runtime thing. If that's easy and not-so-intrusive to cover it with defaults it's fine. If it is something tricky then don't really think it worth the time.

Fix memory leaks with DNA_struct_default_get

Harbormaster completed remote builds in B19925: Diff 46959.Jan 12 2022, 9:56 AM

There is an issue when updating this to master, I am investigating...

Recompiled on current master

Merge master
Fix allocation in node_composite_movieclip.cc

Harbormaster completed remote builds in B19928: Diff 46965.Jan 12 2022, 11:30 AM

Merge branch 'master' into arcpatch-D13367

Harbormaster completed remote builds in B19942: Diff 46989.Jan 12 2022, 5:21 PM

Fix new defaults header not being configured by CMake
Add missing re-enabling of clang-format
Reorder member initialization order to match struct

Harbormaster completed remote builds in B19944: Diff 46991.Jan 12 2022, 5:39 PM

Did some minor tweaks, this seems ready now. Will commit.

This revision is now accepted and ready to land.Jan 12 2022, 5:57 PM

Closed by commit rBbab47b60cb69: DNA: Add space clip editor defaults (authored by Simon Lenz (Amudtogal), committed by Julian Eisel (Severin)). · Explain WhyJan 12 2022, 6:05 PM

This revision was automatically updated to reflect the committed changes.

Julian Eisel (Severin) added a commit: rBbab47b60cb69: DNA: Add space clip editor defaults.

DNA_defaults: Add space clip editor defaults
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 45276

source/blender/editors/space_clip/CMakeLists.txt

source/blender/editors/space_clip/space_clip.c

source/blender/makesdna/DNA_defaults.h

source/blender/makesdna/DNA_movieclip_defaults.h

source/blender/makesdna/DNA_space_defaults.h

source/blender/makesdna/intern/dna_defaults.c

source/blender/nodes/CMakeLists.txt

source/blender/nodes/composite/nodes/node_composite_movieclip.cc

DNA_defaults: Add space clip editor defaultsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 45276

source/blender/editors/space_clip/CMakeLists.txt

source/blender/editors/space_clip/space_clip.c

source/blender/makesdna/DNA_defaults.h

source/blender/makesdna/DNA_movieclip_defaults.h

source/blender/makesdna/DNA_space_defaults.h

source/blender/makesdna/intern/dna_defaults.c

source/blender/nodes/CMakeLists.txt

source/blender/nodes/composite/nodes/node_composite_movieclip.cc

DNA_defaults: Add space clip editor defaults
ClosedPublic
Actions

Revision Contents
Changeset List