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

[3.x] Rationalize property reversion #51166

Merged
merged 4 commits into from
Aug 9, 2021

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Aug 2, 2021

Version of #46270 + #51173 + #51180 for 3.x.

@RandomShaper
Copy link
Member Author

In the cherry-picked commit from 4.0, I've replaced the cases of #warning with WARN_PRINT_ONCE, to avoid errors in the CI. Moreover, given that they are about not-yet-implemented stuff, merging this PR would require tracking the progress of it in 4.0, to backport the missing pieces when they are available.

@akien-mga
Copy link
Member

In the cherry-picked commit from 4.0, I've replaced the cases of #warning with WARN_PRINT_ONCE, to avoid errors in the CI. Moreover, given that they are about not-yet-implemented stuff, merging this PR would require tracking the progress of it in 4.0, to backport the missing pieces when they are available.

This needs to be implemented in this PR to be mergeable IMO. We used #warning in master for things that couldn't reasonably be implemented at the same time as the main feature that caused the need to implement them. But there's a big difference between 4.0.dev in master and 3.4.beta in 3.x in terms of expectations of stability and feature completeness.

@neikeq
Copy link
Contributor

neikeq commented Aug 2, 2021

I was about to comment this but the diff changed and it seems inherits_script is no longer a thing? Here it is any way in case it may be of some use:

I haven't tested this code nor whether this PR breaks anything for C#, but this is probably how inherits_script should be implemented for C#:

bool CSharpScript::inherits_script(const Ref<Script> &p_script) const {
	if (!script_class)
		return false;
	Ref<CSharpScript> cs_script = p_script;
	if (cs_script.is_null())
		return false;
	if (!cs_script->script_class)
		return false;
	return cs_script->script_class->is_assignable_from(script_class);
}

@RandomShaper
Copy link
Member Author

@neikeq, it changed indeed. I was in the middle of writing this comment.

UPDATES:

  • This PR now also includes master's Fix up property reversion #51173.
  • Instead of cherry-picking the Script::inherits_script() work from master, this now includes a simpler implementation, which doesn't include unimplemented functions. @neikeq, do you think it would be possible to write CSharpScript::get_base_script() as of now?

@neikeq
Copy link
Contributor

neikeq commented Aug 2, 2021

It seems the new inherits_script cannot be overridden and relies on get_base_script, which is also not implemented for C#. In fact get_base_script is much harder to implement for C#, so if C# could override inherits_script that would be better.

@RandomShaper
Copy link
Member Author

I see. I'll go back to the former way. Thank you.

RandomShaper and others added 2 commits August 2, 2021 15:18
Partial cherry-pick of 5d4dc2d.

Co-authored-by: Juan Linietsky <[email protected]>
@RandomShaper
Copy link
Member Author

@akien-mga, sorry for the stream of updates.

Since a showstopper for this PR was the lack of many implementations of inherits_script(), I've added the NativeScript one myself and cherry-picked the C# one. So, the only one missing at this point is the PluginScript one, which I don't think is a big issue. (If it is, please let me know, since I'm not very familiar with it and I may be missing some important consequences of that.)

With all the commits included here, this is fully aligned with master and should be ready to merge once #51173 and #51180 are validated.

@akien-mga akien-mga merged commit a418d09 into godotengine:3.x Aug 9, 2021
@akien-mga
Copy link
Member

Thanks!

@RandomShaper RandomShaper deleted the fix_can_reset_3.x branch August 9, 2021 10:00
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.

3 participants