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

Add application/config/enable_uids project setting #81122

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dhoverml
Copy link

@dhoverml dhoverml commented Aug 29, 2023


static ResourceUID *get_singleton() { return singleton; }

ResourceUID();
~ResourceUID();
};

class ResourceUIDDummy : public ResourceUID {
GDCLASS(ResourceUIDDummy, ResourceUID)
Copy link
Member

Choose a reason for hiding this comment

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

You haven't registered this class, without doing so it won't work, you need to use GDREGISTER_ABSTRACT_CLASS like is done with ResourceUID

Copy link
Author

Choose a reason for hiding this comment

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

Nice catch!
Interesting that I ran into no issues while testing this, though I didn't try using ResourceUID in GDScript.
Wonder if this case should be static_assert at compile time, similar to #81020.
Anyway, I've pushed an update.

Copy link
Member

@AThousandShips AThousandShips Aug 29, 2023

Choose a reason for hiding this comment

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

The opposite is impossible unfortunately, and there are many valid cases of not registering a class for when they aren't enabled

Edit: To clarify why I say it isn't possible:
With my changes to check at registration there's a well defined point of failure, when the class is registered, but doing the opposite isn't trivial, how do you define the point when a class hasn't been registered? Arguably you could create a solution having some large list of all classes that have been declared, but it'd be very complicated and would probably be hard or impossible to do without making every header file dependent on every other header file

@AThousandShips
Copy link
Member

You need to document the method is_dummy, the easiest way to do this is run --doctool on the compiled version of your contribution, see here

@dhoverml
Copy link
Author

I was thinking, would it be preferable to modify ResourceUID to always check if the project setting is set (or cache a bool in its constructor)? That way there wouldn't be a new class. Let me know what you prefer.

@AThousandShips
Copy link
Member

It shouldn't check the setting every time, that would be too bad performance wise, but it could load the value on creation (as it is only changed on restart) and then use that value

@dhoverml
Copy link
Author

Okay, I removed ResourceUIDDummy and instead cache the project setting. Also updated the docs.
Also, maybe _is_dummy should be renamed to is_stubbed, though I don't mind either way.

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.

Add project setting to disable UIDs and UID caching
2 participants