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

Fix invalid inheritance of OccluderInstance3D #93354

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Jun 19, 2024

Unsure what the proper procedure is here, and what issues might have come from the current code, but this is a pretty glaring issue at hand here

Marking as compatibility breaking, even though it's just aligning the types to what they are supposed to be

@AThousandShips
Copy link
Member Author

Unlike the check for having a valid GDCLASS this is very hard to fix with macros to ensure it doesn't happen, but will try figure out a way to handle it like in:

@clayjohn
Copy link
Member

Why is this needed? I don't think OccluderInstance needs any of the behaviour of VisualInstance. For clarity, VisualInstance is the root for things that are going to get rendered in 3D.

@AThousandShips
Copy link
Member Author

AThousandShips commented Jun 19, 2024

Why should it differ from it's actual inheritance? See:

class OccluderInstance3D : public VisualInstance3D {

I can change that instead if desired

Note that it does override get_aabb from that class

Edit: or do you mean why the class inherits from there in the first place?

@clayjohn
Copy link
Member

Oh wow, I didn't notice the actual inheritance was from VisualInstance. I wonder what would be less compatibility breaking, changing the GDClass or the actual inheritance

@AThousandShips
Copy link
Member Author

AThousandShips commented Jun 19, 2024

It currently uses at least one functionality from VisualInstance3D, will check what the editor management is designed based on and see if it also assumes this inheritance

There's nothing obviously implying VisualInstance3D in the editor code, but I haven't tested changing the actual class and seeing if it breaks anything as I don't use this class myself, someone more experienced with it would be better to do that

@AThousandShips
Copy link
Member Author

It does indeed need to be a visual instance, it uses set_base

@raulsntos
Copy link
Member

Regarding C# compat, this PR doesn't break compat for us because the new base class VisualInstance3D derives from the previous base class Node3D. So it's just inserting a new base class in between which is OK in C# as long as it doesn't introduce new abstract members.

@AThousandShips
Copy link
Member Author

That's very true! Forgot that part, so I'll remove that aspect as it doesn't actually break compatibility in any real way I'd say

@akien-mga akien-mga modified the milestones: 4.4, 4.3 Jun 19, 2024
@akien-mga akien-mga merged commit 8f739cc into godotengine:master Jun 20, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the fix_occluder_hierarchy branch June 20, 2024 14:27
@AThousandShips
Copy link
Member Author

Thank you!

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.

4 participants