Collection Export: Allow renaming of exporters in the UI list #125553

Jesse Yurkovich · 2024-07-28T02:38:12+02:00

Jesse Yurkovich commented

2024-07-28 02:38:12 +02:00

Allow the user to rename their collection exporters.

Jesse Yurkovich force-pushed collexport-renaming from 8709203920 to cd5aea6191

2024-07-30 02:36:41 +02:00

Compare

Jesse Yurkovich requested review from Bastien Montagne 2024-07-30 19:13:39 +02:00

Bastien Montagne requested changes 2024-07-31 09:53:51 +02:00

Bastien Montagne left a comment

I would rather move the logic in rna_CollectionExport_friendly_name into the CollectionExporter code (aka collection_exporter_add_exec). that way, the name is defined as part of creation process, instead of using some random accessor code. ;)

Then you can probably get rid of all RNA callbacks for this property, since it becomes just a standard DNA-defined fixed-size char buffer.

There is also the question of whether this name should be unique or not? I would say yes, since that allows to reference the collection exporters data by their name. Just like we do e.g. with constraints or modifiers (and of course IDs themselves).

The following may be going out of scope for this PR, and could be addressed separately. But I think it would be good to also move the core creation code of CollectionExporter into BKE code (there is already a BKE_collection_exporter_free_data there). That way, other areas of code can easily create this data (thinking mainly about a RNA API to create and remove these exporters).

I would rather move the logic in `rna_CollectionExport_friendly_name` into the CollectionExporter code (aka `collection_exporter_add_exec`). that way, the name is defined as part of creation process, instead of using some random accessor code. ;) Then you can probably get rid of all RNA callbacks for this property, since it becomes just a standard DNA-defined fixed-size char buffer. There is also the question of whether this name should be unique or not? I would say yes, since that allows to reference the collection exporters data by their name. Just like we do e.g. with constraints or modifiers (and of course IDs themselves). ------------ The following may be going out of scope for this PR, and could be addressed separately. But I think it would be good to also move the core creation code of `CollectionExporter` into BKE code (there is already a `BKE_collection_exporter_free_data` there). That way, other areas of code can easily create this data (thinking mainly about a RNA API to create and remove these exporters).

Jesse Yurkovich added 2 commits 2024-07-31 19:03:05 +02:00

Merge remote-tracking branch 'upstream/main' into collexport-renaming 04dbca56a1

Feedback: less reliance on RNA getters/setters 13cdb14d07

Jesse Yurkovich commented

2024-07-31 19:05:04 +02:00

Fair feedback, thanks! I did leave the RNA setter because I need to prevent assignment of completely empty names as that seems like something reasonable to do. I'll keep the last part of the feedback in mind if/when more changes in the area occur.

Bastien Montagne requested changes 2024-07-31 19:16:35 +02:00

Bastien Montagne left a comment

Would suggest adding a BKE_collection_exporter_name_set function, and call it from the everywhere (doversion, add operator, name set RNA callback)? Avoids duplicating the same logic everywhere, with the usual risk of getting it out of sync at some point in the future.

Would suggest adding a `BKE_collection_exporter_name_set` function, and call it from the everywhere (doversion, add operator, name set RNA callback)? Avoids duplicating the same logic everywhere, with the usual risk of getting it out of sync at some point in the future.

source/blender/blenloader/intern/versioning_400.cc Outdated

						
				@ -4448,0 +4454,4 @@

				    LISTBASE_FOREACH (Collection *, collection, &bmain->collections) {

				      ListBase *exporters = &collection->exporters;

				      LISTBASE_FOREACH (CollectionExport *, data, exporters) {

				        if (data->name[0] == '\0') {

Bastien Montagne commented

2024-07-31 19:14:38 +02:00

You need to call BLI_uniquename here too, even if the name is not empty

You need to call `BLI_uniquename` here too, even if the name is not empty

Jesse Yurkovich commented

2024-07-31 20:32:41 +02:00

The lastest commit now calls BLI_uniquename as part of the processing but I think the check for data->name being empty is fine? This should only be the case during versioning unless I'm missing a scenario perhaps?

The lastest commit now calls `BLI_uniquename` as part of the processing but I think the check for data->name being empty is fine? This should only be the case during versioning unless I'm missing a scenario perhaps?

Bastien Montagne commented

2024-08-02 11:01:44 +02:00

Hmmm in fact since you are bumping subversion number, I think you can drop the if block and replace it by an assert that the name is empty?

Hmmm in fact since you are bumping subversion number, I think you can drop the `if` block and replace it by an assert that the name _is_ empty?

deadpin marked this conversation as resolved

source/blender/makesrna/intern/rna_collection.cc Outdated

						
				@ -475,1 +459,3 @@

				  return strlen(value);

				  /* Only set the name if it's not empty. */

				  if (value[0] != '\0') {

				    STRNCPY(data->name, value);

Bastien Montagne commented

2024-07-31 19:14:12 +02:00

You need to call BLI_uniquename here as well.

You need to call `BLI_uniquename` here as well.

deadpin marked this conversation as resolved

Jesse Yurkovich added 1 commit 2024-07-31 20:31:24 +02:00

Feedback: BKE_collection_exporter_name_set 307b61f907

Bastien Montagne approved these changes 2024-08-02 11:09:31 +02:00

Bastien Montagne left a comment

LGTM now, added some notes below, don't think it needs further review once they are addressed.

source/blender/blenkernel/intern/collection.cc Outdated

						
				@ -516,6 +517,18 @@ void BKE_collection_free_data(Collection *collection)

				  collection_free_data(&collection->id);

				}

				void BKE_collection_exporter_name_set(CollectionExport *data, const char *newname)

Bastien Montagne commented

2024-08-02 11:08:29 +02:00

Doing a pointer look up in the listbase is not really expensive, especially when the list is expected to be small, but I would pass the owner listbase as a parameter here (handling cases where it's nullptr). Calling code knows it in two cases over three already, and hopefully once #122431 is finished the RNA code will have access to this info too.

Doing a pointer look up in the listbase is not really expensive, especially when the list is expected to be small, but I would pass the owner listbase as a parameter here (handling cases where it's `nullptr`). Calling code knows it in two cases over three already, and hopefully once #122431 is finished the RNA code will have access to this info too.

deadpin marked this conversation as resolved

Jesse Yurkovich added 2 commits 2024-08-02 21:53:29 +02:00

Merge remote-tracking branch 'upstream/main' into collexport-renaming 6359337974

Feedback: use owning ListBase when available

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details