GPv3: Refactor root_group to be a pointer in DNA #110007

Falk David · 2023-07-12T12:37:44+02:00

Falk David commented

2023-07-12 12:37:44 +02:00

This turns the embedded group into a pointer.

Note: This will break files from opening if they have a OB_GREASE_PENCIL in them!

This turns the embedded group into a pointer. Note: This will break files from opening if they have a `OB_GREASE_PENCIL` in them!

Falk David added 1 commit 2023-07-12 12:37:54 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

d134f98aff GPv3: Refactor root_group to be a pointer in DNA

Falk David commented

2023-07-12 12:40:01 +02:00

@blender-bot build

Falk David requested review from Bastien Montagne 2023-07-12 12:40:11 +02:00

Falk David commented

2023-07-12 12:41:00 +02:00

@mont29 Should I try to do versioning this change? Or is it ok to break files that use the experimental feature?

Bastien Montagne commented

2023-07-12 12:56:56 +02:00

@filedescriptor no strict need for versioning changes in experimental features no... Nice to have if possible and easy, otherwise just let things break ;)

Bastien Montagne commented

2023-07-12 13:04:30 +02:00

Not sure why opening older files would break though? They should just show as an empty GP?

Bastien Montagne approved these changes 2023-07-12 13:04:57 +02:00

Bastien Montagne left a comment

From a quick check changes look fine.

Falk David commented

2023-07-12 17:21:56 +02:00

@mont29 It crashes because the root_group pointer can't be read (so it's nullptr), but all the code assumes that there always is a root group. The active_layer pointer still points to valid stuff though.
I could allocate an empty group if root_group could not be read I guess, but then accessing the active_layer pointer will crash anyway.
And just clearing it will work, but then leak memory...

I think technically you could reconstruct the whole thing with the active layer pointer, but it seems not worth it to do that.

@mont29 It crashes because the `root_group` pointer can't be read (so it's nullptr), but all the code assumes that there always is a root group. The `active_layer` pointer still points to valid stuff though. I could allocate an empty group if `root_group` could not be read I guess, but then accessing the active_layer pointer will crash anyway. And just clearing it will work, but then leak memory... I think technically you could reconstruct the whole thing with the active layer pointer, but it seems not worth it to do that.

Bastien Montagne commented

2023-07-12 19:15:40 +02:00

@mont29 It crashes because the root_group pointer can't be read (so it's nullptr), but all the code assumes that there always is a root group. The active_layer pointer still points to valid stuff though.
I could allocate an empty group if root_group could not be read I guess, but then accessing the active_layer pointer will crash anyway.
And just clearing it will work, but then leak memory...

I think technically you could reconstruct the whole thing with the active layer pointer, but it seems not worth it to do that.

I would at least generate an empty root_group then, and clear (and free) active_layer. Not conserving data from experimental feature is okay, but would really try to avoid any crash.

> @mont29 It crashes because the `root_group` pointer can't be read (so it's nullptr), but all the code assumes that there always is a root group. The `active_layer` pointer still points to valid stuff though. > I could allocate an empty group if `root_group` could not be read I guess, but then accessing the active_layer pointer will crash anyway. > And just clearing it will work, but then leak memory... > > I think technically you could reconstruct the whole thing with the active layer pointer, but it seems not worth it to do that. I would at least generate an empty `root_group` then, and clear (and free) `active_layer`. Not conserving data from experimental feature is okay, but would really try to avoid any crash.

Bastien Montagne requested changes 2023-07-12 19:16:10 +02:00

Bastien Montagne left a comment

Think crash should be avoided at least.

Falk David added 2 commits 2023-07-13 14:50:57 +02:00

61333151b0 Merge branch 'main' into gpv3-refactor-root-group

6596dedaa4 Avoid crashing when loading older files

Falk David requested review from Bastien Montagne 2023-07-13 14:51:17 +02:00

Falk David added 1 commit 2023-07-13 17:00:37 +02:00

7500749616 Simplify `read_layer_tree` function

Falk David added 1 commit 2023-07-13 17:03:44 +02:00

4f45e10859 Update comment

Bastien Montagne approved these changes 2023-07-13 17:22:01 +02:00

Bastien Montagne left a comment

LGTM now.

Falk David merged commit f2f0426980 into main

2023-07-13 18:24:27 +02:00

Falk David referenced this issue from a commit

2023-07-13 18:24:27 +02:00

GPv3: Refactor root_group to be a pointer in DNA

Falk David deleted branch gpv3-refactor-root-group

2023-07-13 18:24:28 +02:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

GPv3: Refactor root_group to be a pointer in DNA #110007