Page MenuHome

DNA renaming support (forward compatible)
ClosedPublic

Authored by Campbell Barton (campbellbarton) on Feb 12 2019, 1:49 PM.

Details

Summary

This patch supports forward compatible renaming of structs and struct members.

Tested the patch to rename:

  • Lamp to Light
  • Light.clipsta to Light.clip_start
  • Camera.YF_dofdist to Camera.dof_dist.
  • SpaceOops to SpaceOutliner.

See: T61500 for proposed changes.

Blend files saves with this renaming load with the patch applied or not.

However these changes aren't included in this patch, instead the patch includes a utility, run:

python3 version_update_D4342_utility.py

How it Works

Once a struct or struct member is renamed the old name only needs to be referenced once in dna_rename_defs.h, this causes the SDNA written to disk to use the old name.

Only makesdna and makesrna need to handle this, so it's all managed at build time.

Details

  • No run-time penalty since all versioning happens in makesdna and makesrna.
  • Collisions caused by renaming aren't allowed (a makesdna error for struct members, structs themselves cause a duplicate enum - also failing).
  • The changes in dna_rename_defs.h are just suggestions & to test renaming works.
  • All versioning is done in dna_rename_defs.h (nice to keep DNA versioning in the one place).

Current naming conventions:

  • struct_* the name of a struct.
  • elem_* a struct member.
  • *_static name of data stored on disk (changes from dna_rename_defs.c applied).
  • *_runtime name of data used at runtime (names used in DNA_*.h headers).
  • _trimmed variable names without pointer/array sizes, eg *var[4] would be trimmed to var.
  • DNA_alias_* is used for the API, since this isn't renaming DNA, the original members still exist and can't be used again.

Diff Detail

Repository
rBS Blender Staging
Branch
temp-dna-rename
Build Status
Buildable 2940
Build 2940: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Could only do some quick sanity checking, without going too deep into implementation details (not really my area, so might be missing something).

source/blender/blenloader/intern/versioning_dna.c
34

Does it have to be a single file? As in, what prevents it from being split?

Is rather annoying to work on such files in a real IDE, since they don't know what is the state of DNA_MAKESDNA you intend it to be to do proper code highlight.

source/blender/blenloader/intern/versioning_dna.c
34

This could be exposed differently, eg, at the bottom of each DNA_*.h file we could include a section, or even auto-detect some defines after the struct declaration.

I mostly wanted to avoid things getting out of control with renaming of the same data happening in different places, in a way that might be non-obvious what the final result is.

If we want to change this we can w/o too much hassles afaics.

source/blender/blenloader/intern/versioning_dna.c
34

Spreading all over is an overkill, But having one file for "dangerous" and another one for "safe" renames seems reasonable to me.

source/blender/blenloader/intern/versioning_dna.c
34

We could split this in two files, I don't especially mind - but cant ignore that operating on same data in two places, could backfire.

In practice we'll probably only rarely use non-forward-compatible versioning - so it might not be so risky.

source/blender/blenloader/intern/versioning_dna.c
34

Having two absolutely different ways of do-versioning in the same file can easily backfire as well.

I would prefer two files, clearly describing what they are for (as opposite of making an extra decision on DNA_MAKESDNA which makes no sense from the point of view of a developer who wants to fix some naming).

The "dangerous" file then can have BIG WARNING WITH 72PT FONT TELLING THAT ONE ENTERS A DANGER ZONE!!!1111ONEONEONE

  • Split out two kinds of versioning
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
  • Correct renaming script (didn't skip the renamed file)
Harbormaster completed remote builds in B2922: Diff 13664.
source/blender/blenloader/intern/versioning_dna.c
34

Moved to dna_softpatch_defs.h since it's arguably not versioning.

  • Function to strip DNA names in-place (simplify duplicate check a little)
  • Stress test - ensure matrix works
  • Correct DNA naming error (result was stripped)
Harbormaster completed remote builds in B2928: Diff 13672.
  • Simplify creating runtime names
  • Simplify stripping the name in makesdna.
  • Note that renaming is only for test, not part of patch

@Campbell Barton (campbellbarton), yay for splitting the file!

Not sure, anything specifically you expect to be still reviewed/tested?

source/blender/makesdna/intern/dna_softpatch_defs.h
45 ↗(On Diff #13678)

I wouldn't mind if those are committed ;)

49 ↗(On Diff #13678)

Is YF yo frankie? =\

Brecht Van Lommel (brecht) requested changes to this revision.Feb 14 2019, 4:16 PM

I tried to use this for the Group to Collection renaming hack: P913. But that leads to lots of errors in the generate RNA code. Not sure if I did something wrong or if it exposes a bug in the patch.

I'm a little surprised by the amount of code in this patch, given how simple the old hack was. I guess renaming members makes it more complicated?

source/blender/blenloader/intern/versioning_dna.c
19

written -> written with

source/blender/makesdna/intern/dna_softpatch_defs.h
1 ↗(On Diff #13678)

This file could perhaps be located in makesdna/DNA_renames.h

49 ↗(On Diff #13678)

Yafray!

source/blender/makesdna/intern/dna_utils.c
360

source/blender/makesdna/intern/dna_utils.c:266:18: warning: ‘struct_map_local’ may be used uninitialized in this function [-Wmaybe-uninitialized]

This revision now requires changes to proceed.Feb 14 2019, 4:16 PM

I updated P913 so it at least builds, but still crash on startup due to scene->master_collection being NULL. Not sure where exactly that goes wrong, I would expect it to work.

I look into simplifying the RNA code but I can't see an easy way to do it, the old version patching hack didn't handle a bunch of cases so not sure it can be much simpler than this.

source/blender/makesrna/intern/rna_define.c
380

source/blender/makesrna/intern/rna_define.c:376:5: warning: ‘structnr’ may be used uninitialized in this function [-Wmaybe-uninitialized]

  • Fix accessing runtime struct names

@Brecht Van Lommel (brecht) looked into this,

First there was an issue with rna accessing the runtime struct names which didn't show up for me because nothing references 'Light' pointers.
Committed fix (there is no need for ghash lookups since there is an array of runtime names).


As for the Screen/Group/GroupObjects issue, the reason Scene.master_collection is null because the current versioning is incomplete, internally DNA stores Collection *master_collection, not Group *master_collection.

As far as I can see, there isn't much we can do here if we want to be forward compatible or at least forward compatible with existing 2.8x versions.

... unless we add an option for DNA versioning that knows to rename structs but have members that refer to it using the new name.

Will look into this but would avoid if it makes internals too messy.


Having seen how struct names can leak into DNA, I've double checked that names from this patch aren't written (renamed WorkSpace, checked resulting DNA and the string isn't found).

  • Quiet warnings
  • Name dna_rename_defs.h
Campbell Barton (campbellbarton) added inline comments.
source/blender/makesdna/intern/dna_softpatch_defs.h
1 ↗(On Diff #13678)

Rather keep this separate from DNA headers because they don't have to care about the old names (except for collisions), renamed to intern/dna_rename_defs.h.

  • update versioning script to ignore new name
Harbormaster completed remote builds in B2938: Diff 13684.

Use term 'alias' instead of 'softpatch'

Seems more correct then version/patch/rename since it
implies the original name isn't entirely removed and the data isn't modified.

Harbormaster completed remote builds in B2940: Diff 13687.

Looks good to me now.

This revision is now accepted and ready to land.Feb 15 2019, 6:53 PM