WIP: GPv3: Convert legacy objects on file load #108671

Falk David · 2023-06-06T17:58:51+02:00

Falk David commented

2023-06-06 17:58:51 +02:00

This converts the objects of type OB_GPENCIL_LEGACY to the new OB_GREASE_PENCIL type.

'Fixes' an issue where the "2D Animation" template would not behave as expected when using the "Grease Pencil 3.0" experimental option. Users found it to be confusing that the template would load with the old object type (and thus making it not possible to use at all).
Of course this will convert all the legacy grease pencil objects to the new type, but we might as well start doing this now, rather than later.

Currently I am unsure if the little BKE_reportf warning at the bottom of the screen is enough, since this will definitely break files. It is experimental yes, but if users forget to turn off the flag, they might accidentally corrupt files.

This converts the objects of type `OB_GPENCIL_LEGACY` to the new `OB_GREASE_PENCIL` type. 'Fixes' an issue where the "2D Animation" template would not behave as expected when using the "Grease Pencil 3.0" experimental option. Users found it to be confusing that the template would load with the old object type (and thus making it not possible to use at all). Of course this will convert all the legacy grease pencil objects to the new type, but we might as well start doing this now, rather than later. Currently I am unsure if the little `BKE_reportf` warning at the bottom of the screen is enough, since this will definitely break files. It is experimental yes, but if users forget to turn off the flag, they might accidentally corrupt files.

Falk David added 1 commit 2023-06-06 17:58:59 +02:00

1cb0c42d87 Convert legacy grease pencil to new data type on load

Falk David added 1 commit 2023-06-06 18:01:49 +02:00

e318ffd430 Update mode

Falk David requested review from Brecht Van Lommel 2023-06-06 18:03:14 +02:00

Falk David requested review from Dalai Felinto 2023-06-06 18:03:14 +02:00

Falk David requested review from Bastien Montagne 2023-06-06 18:03:14 +02:00

Falk David changed title from ~~GPv3: Convert legacy objects on file load~~ to WIP: GPv3: Convert legacy objects on file load

2023-06-06 18:08:19 +02:00

Falk David added this to the Grease Pencil project 2023-06-06 18:13:08 +02:00

Bastien Montagne requested changes 2023-06-06 18:23:42 +02:00

Bastien Montagne left a comment

There are several issues with this patch, but not even sure it's the way to solve your issue?

Even though you do need conversion at some point, I would only enable it when it has been hard-tested and validated.

Why not rather have a GPv3 '2D Animation" template, and hide one or another depending on whether GPv3 experimental feature or not.

There are several issues with this patch, but not even sure it's the way to solve your issue? Even though you do need conversion at some point, I would only enable it when it has been hard-tested and validated. Why not rather have a GPv3 '2D Animation" template, and hide one or another depending on whether GPv3 experimental feature or not.

source/blender/blenkernel/intern/blendfile.cc

						
				@ -899,1 +901,4 @@

				  /* Convert all `OB_GPENCIL_LEGACY` to `OB_GREASE_PENCIL`. */

				  if (mode != LOAD_UNDO && U.experimental.use_grease_pencil_version3 &&

				      !blendfile_or_libraries_versions_atleast(bmain, 400, 3))

Bastien Montagne commented

2023-06-06 18:14:58 +02:00

This is most likely wrong, since user can still produce 'old' GP data currently in main?

source/blender/blenkernel/intern/blendfile.cc

						
				@ -900,0 +903,4 @@

				  if (mode != LOAD_UNDO && U.experimental.use_grease_pencil_version3 &&

				      !blendfile_or_libraries_versions_atleast(bmain, 400, 3))

				  {

				    LISTBASE_FOREACH (Object *, ob, &bmain->objects) {

Bastien Montagne commented

2023-06-06 18:16:11 +02:00

Thish whole block should be its own function in GP BKE code, not exposed like that in generic readfile code.

source/blender/blenkernel/intern/blendfile.cc

						
				@ -900,0 +917,4 @@

				        blender::bke::greasepencil::convert::legacy_gpencil_to_grease_pencil(

				            *bmain, *new_grease_pencil, *gpd);

				        id_us_min(&gpd->id);

Bastien Montagne commented

2023-06-06 18:19:36 +02:00

You probably want to delete these old bGPdata IDs when they have no more users.

Another thing to consider here is what to do in case more than one GP object use the same bGPdata obdata.

In fact, I think what you should rather do here is loop over bGPdata IDs, make a new version of each, remap ID pointers, and then go over all GP objects to update their types?

You probably want to delete these old bGPdata IDs when they have no more users. Another thing to consider here is what to do in case more than one GP object use the same bGPdata obdata. In fact, I think what you should rather do here is loop over bGPdata IDs, make a new version of each, remap ID pointers, and then go over all GP objects to update their types?

Falk David commented

2023-06-06 18:27:48 +02:00

Why not rather have a GPv3 '2D Animation" template, and hide one or another depending on whether GPv3 experimental feature or not.

Yes that could be an option depending on whether or not we want the automatic conversion for testing. Since it should only do the conversion when the experimental flag is set, I think we should not necessarily wait until it's well tested.

> Why not rather have a GPv3 '2D Animation" template, and hide one or another depending on whether GPv3 experimental feature or not. Yes that could be an option depending on whether or not we want the automatic conversion for testing. Since it should only do the conversion when the experimental flag is set, I think we should not necessarily wait until it's well tested.

Brecht Van Lommel commented

2023-06-06 18:31:06 +02:00

I think doing this automatically when the experimental option is enabled is rather dangerous? I would do this only in versioning_defaults.cc, so that it only affects the startup files, not existing .blend files where it may be too late to notice the mistake.

Alternative, you could have a button in the 3D viewport header or something to do it, if the active object is a legacy grease pencil object.

I think doing this automatically when the experimental option is enabled is rather dangerous? I would do this only in `versioning_defaults.cc`, so that it only affects the startup files, not existing .blend files where it may be too late to notice the mistake. Alternative, you could have a button in the 3D viewport header or something to do it, if the active object is a legacy grease pencil object.

Falk David commented

2023-06-07 17:24:22 +02:00

I will put this on hold for now and wait till we have more stable conversions.

Brecht Van Lommel refused to review 2023-06-14 19:07:36 +02:00

Brecht Van Lommel commented

2023-06-14 19:07:59 +02:00

Removing from my review queue then until this is meant to be reviewed.

Falk David commented

2024-03-28 12:45:53 +01:00

Replaced by !118705. Closing.

Falk David closed this pull request

2024-03-28 12:45:55 +01:00

Pull request closed

Please reopen this pull request to perform a merge.

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

WIP: GPv3: Convert legacy objects on file load #108671

Pull request closed