X Tutup
Skip to content

Replace many uses of is_class with derives_from. (reverted)#110832

Merged
Repiteo merged 1 commit intogodotengine:masterfrom
Ivorforce:is-class-to-derives-from
Sep 24, 2025
Merged

Replace many uses of is_class with derives_from. (reverted)#110832
Repiteo merged 1 commit intogodotengine:masterfrom
Ivorforce:is-class-to-derives-from

Conversation

@Ivorforce
Copy link
Member

@Ivorforce Ivorforce commented Sep 23, 2025 •
edited
Loading

is_class is very slow: It necessitates allocating a String, and de-referencing many other strings to check against. This induces many RAM cache misses on cold access. It is also suboptimally implemented, making the same test several times in subclasses.
In addition, is_class calls use string literals, which are error prone and don't support the "Go to" function of IDEs for quick navigation.
Finally, is_class uses odd type checking logic, which e.g. checks GDExtension types but ignores scripting types, and will likely be phased out eventually.

This PR uses the new derives_from function, which is at least several times faster than is_class, in much of the codebase.

I essentially used a regex replace (and deleted changes that led to compile errors, e.g. due to forward declarations):

is_class\("(\w+)"\)
derives_from<$1>()

This PR reduces the number of is_class calls within the engine by about 2/3.

@Ivorforce Ivorforce requested review from a team as code owners September 23, 2025 17:44
@Ivorforce Ivorforce requested review from a team September 23, 2025 17:44
@Ivorforce Ivorforce requested a review from a team as a code owner September 23, 2025 17:44
@Ivorforce Ivorforce requested a review from a team September 23, 2025 17:44
@Ivorforce Ivorforce requested review from a team as code owners September 23, 2025 17:44
@Ivorforce Ivorforce requested review from lawnjelly and removed request for a team September 23, 2025 17:45
@Ivorforce Ivorforce added this to the 4.x milestone Sep 23, 2025
@Ivorforce Ivorforce force-pushed the is-class-to-derives-from branch from 2bd6e45 to eb1b616 Compare September 23, 2025 17:48
@Ivorforce Ivorforce force-pushed the is-class-to-derives-from branch from eb1b616 to 8ef4a43 Compare September 23, 2025 17:59
@lawnjelly
Copy link
Member

lawnjelly commented Sep 23, 2025

Are there any implications for removing the GDExtension stuff?

@Ivorforce
Copy link
Member Author

Ivorforce commented Sep 23, 2025

Are there any implications for removing the GDExtension stuff?

No. If we were checking for a GDExtension class with the call, it would not compile, because there would be no symbol for it.

@akien-mga
Copy link
Member

akien-mga commented Sep 23, 2025 •
edited
Loading

For the record, one use case for is_class is to workaround tight coupling between classes and avoid having to include a header to resolve the class name. This makes it possible e.g. in core to do obj->is_class("Node") without having to include scene/main/node.h (which would break core encapsulation).

None of the replacements here seem to be that case though so it's fine. We'd usually favor Object::cast_to<T>() over is_class() for these, but if we have now derives_from<T> that's probably better semantics.

@Ivorforce
Copy link
Member Author

Ivorforce commented Sep 23, 2025

This makes it possible e.g. in core to do obj->is_class("Node") without having to include scene/main/node.h (which would break core encapsulation).

I want to raise that Core classes checking for non-Core classes still breaks encapsulation. It just doesn't break it on an include level. It's still worth avoiding this situation, even with is_class.

lawnjelly
lawnjelly approved these changes Sep 23, 2025
View reviewed changes
Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

Looks fine but I haven't tested, am assuming @Ivorforce has done some testing for regressions.

@Ivorforce
Copy link
Member Author

Ivorforce commented Sep 23, 2025

Looks fine but I haven't tested, am assuming @Ivorforce has done some testing for regressions.

I ran the unit tests, but other than that, no. I don't expect any risks from this PR. The regex cases should be perfectly exchangeable.

@Repiteo Repiteo modified the milestones: 4.x, 4.6 Sep 23, 2025
@Repiteo Repiteo merged commit 78b743c into godotengine:master Sep 24, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Sep 24, 2025

Thanks!

@Ivorforce Ivorforce deleted the is-class-to-derives-from branch September 24, 2025 15:06
This was referenced Sep 24, 2025
Closed
Closed
AThousandShips
AThousandShips reviewed Sep 25, 2025
View reviewed changes
scene/debugger/scene_debugger.cpp

if (keep_transform) {
if (n2->is_class("Node3D")) {
if (n2->derives_from<Node3D>()) {
Copy link
Member

@AThousandShips AThousandShips Sep 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This, and probably many other cases, breaks 2D only builds, will make a report, but I think we should just roll this back and rework it as it's a serious breakage

Will test enclosing these cases in _3D_DISABLED and see that should fix it, but it makes it clear that this kind of change is a bit unstable

Some specific cases might still break in certain configurations if the relevant classes aren't available though

@AThousandShips AThousandShips mentioned this pull request Sep 25, 2025
Closed
@AThousandShips
Copy link
Member

AThousandShips commented Sep 25, 2025

I'll just go ahead and make a revert PR for this for now, it's far too fragile to fix, we can retry this if it can be fixed properly later

AThousandShips added a commit to AThousandShips/godot that referenced this pull request Sep 25, 2025
@AThousandShips
Revert "Merge pull request godotengine#110832 from Ivorforce/is-class…
25c73a1 
…-to-derives-from"

This reverts commit 78b743c, reversing
changes made to a7b2cd6.
@AThousandShips AThousandShips mentioned this pull request Sep 25, 2025
Merged
@Repiteo Repiteo changed the title Replace many uses of is_class with derives_from. Replace many uses of is_class with derives_from. (reverted) Sep 25, 2025
@Yarwin Yarwin mentioned this pull request Oct 22, 2025
Open
@KoBeWi
Copy link
Member

KoBeWi commented Dec 25, 2025

Did the usage of derives_from() change in the meantime? p_object->derives_from<CanvasItem>() no longer compiles, some template error about O argument.

@Ivorforce
Copy link
Member Author

Ivorforce commented Dec 25, 2025

Yes, as a temporary measure. See #113152

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

Reviewers

@AThousandShips AThousandShips AThousandShips left review comments

@lawnjelly lawnjelly lawnjelly approved these changes

Assignees

No one assigned

Labels

enhancement performance topic:core

Projects

None yet

Milestone

4.6

Development

Successfully merging this pull request may close these issues.

6 participants

@Ivorforce @lawnjelly @akien-mga @Repiteo @AThousandShips @KoBeWi
X Tutup