Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark underscored properties as internal #94954

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

raulsntos
Copy link
Member

These properties look like they were intended to be internal but they were missing the PROPERTY_USAGE_INTERNAL flag.

  • PackedScene::_bundled
  • PortableCompressedTexture2D::_data
  • ImporterMesh::_data

These properties look like they were intended to be internal but they were missing the `PROPERTY_USAGE_INTERNAL` flag.

- `PackedScene::_bundled`
- `PortableCompressedTexture2D::_data`
- `ImporterMesh::_data`
@raulsntos raulsntos added this to the 4.x milestone Jul 30, 2024
@raulsntos raulsntos requested a review from a team as a code owner July 30, 2024 17:56
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These cases make sense to me, and I trust the C# side with respect to any possible compatibility, unsure how these are handled there (as this doesn't affect GDScript in any major way, beyond hiding it)

How does this affect extensions? Does the flag affect anything or is the _ already the controlling factor?

CC @dsnopek

@raulsntos
Copy link
Member Author

C# also only hides them, so it doesn't break compatibility.

How does this affect extensions? Does the flag affect anything or is the _ already the controlling factor?

Yes, these properties are already excluded from extension_api.json because they are prefixed by _ (see #64429). Also, AFAIK godot-cpp doesn't generate properties anyway.

@AThousandShips
Copy link
Member

Can confirm the API dump skips them already, and it doesn't touch the getter or setter so it won't change anything, so looks good all around

@Mickeon
Copy link
Contributor

Mickeon commented Jul 30, 2024

While I do agree in principle, I would be very careful with _bundled. It has been "public" for a really, really long time (even in 3.x).
Documented roughly, it's technically functional, and it's the main thing the reader will see when looking at PackedScene's documentation. Someone, somewhere out there is relying on this property, which will... not be unexposed, per say, but will suddenly look like it has disappeared.

In my opinion it should be deprecated in favour of using SceneState, instead.

However, if SceneState is "less" comfortable to access than the dictionary representation, this property has some reason to be exposed.

@AThousandShips
Copy link
Member

I'd say documenting it was a mistake and it should either have been called bundled or not been documented, the impact should be minimal and I'd say to maybe just move the description of _bundled to the main description of the class if we consider it critical to not lose that information

@Calinou
Copy link
Member

Calinou commented Jul 31, 2024

OptimizedTranslation also has those properties which don't any make sense when viewed in the inspector:

image

Should these be marked as internal?

Testing project: test_csv_translation.zip

@raulsntos
Copy link
Member Author

@Calinou If you just want to hide them in the inspector we can use PROPERTY_USAGE_NO_EDITOR. If you also want to hide them from scripting, then we would also use PROPERTY_USAGE_INTERNAL.

However, these properties are added with _get_property_list which means they are already somewhat internal, since properties added this way aren't documented in the class reference and they won't be generated for the C# bindings. So marking them internal probably doesn't do much.

@akien-mga akien-mga merged commit 78935ca into godotengine:master Aug 27, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@raulsntos raulsntos deleted the mark-internal-properties branch August 28, 2024 00:13
@BlueCube3310
Copy link
Contributor

BlueCube3310 commented Aug 28, 2024

This PR causes a regression on the master branch as subresources of nodes are no longer saved, adding either the PROPERTY_USAGE_STORAGE or PROPERTY_USAGE_NO_EDITOR flag seems to fix it.

@Zireael07
Copy link
Contributor

... and another regression where FBX/GLTF don't instance

@Mickeon
Copy link
Contributor

Mickeon commented Aug 28, 2024

I think it's really funny that the best way to have people test PRs is to force them to in a dev build, like throwing spaghetti at the wall to see if they stick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants