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

More work on porting to Godot4 -- Part 2 #3

Merged
merged 3 commits into from
Mar 7, 2023
Merged

More work on porting to Godot4 -- Part 2 #3

merged 3 commits into from
Mar 7, 2023

Conversation

limbonaut
Copy link

@limbonaut limbonaut commented Mar 6, 2023

I've marked some warnings to be ignored - particularly those that complain about overriding native class declaration. It seems intentional, since those methods are later used in other places of the plugin.

@mphe
Copy link
Owner

mphe commented Mar 6, 2023

There is a reason why "overriding native class declaration" warning exists and why it is treated as error by default.
That is because not all engine methods are marked virtual and therefore not overridable. Instead they get shadowed. That means they will not necessarily get called as you would expect. This behavior already existed in 3.x but Godot didn't emit a warning about it.
See also the discussion in godotengine/godot#55024.

So, ignoring those warnings is not a fix. These cases of shadowing need to be actually fixed by changing the implementation.
I saw you already fixed some of it, e.g. the part for adding child nodes. So there are only a few cases left where we need to find a better solution.

Also, the duplicate() overrides in various files are most certainly not necessary anymore, since godotengine/godot#58031 was fixed.

I guess it's better to split this PR up in multiple parts. I will create some issues to keep track of those parts that need more sophisticated fixes.

@mphe
Copy link
Owner

mphe commented Mar 6, 2023

Apart from hiding those override warnings, I like the rest and would merge it, if you separate it.

Notable changes:
- Fix signal names having same names as methods which is no longer allowed
- Fix 3.x API still used in places
- Fix member declarations and add types
- Fix editor popup showing on top of edge material panel
- Fix position of edge material panel
Just as the other editor tool buttons.
@limbonaut
Copy link
Author

Alright, I left those warnings as is. Fixed another case of shadowing in one of the panels.

There's one more thing. It seems that Z Index section is hidden in GUI edge panel and remains unused. I checked with upstream, and it seems to be the case there. I left it as is for now.

@mphe mphe merged commit 4687822 into mphe:gd4 Mar 7, 2023
@mphe
Copy link
Owner

mphe commented Mar 7, 2023

Merged, good work!

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

Successfully merging this pull request may close these issues.

2 participants