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

[WIP] Godot4 (GDScript2) Support #95

Closed
wants to merge 33 commits into from

Conversation

elgansayer
Copy link
Contributor

This is NOT ready. This is work in progress

This is simply a placeholder to force a conversation and gather contributors/conversations to help port this to gdscript2.

Feel free to close if not allowed.

* Update getsets to properties
* Update yields -> await
* update super
@CLAassistant
Copy link

CLAassistant commented Mar 18, 2022

CLA assistant check
All committers have signed the CLA.

@elgansayer elgansayer changed the title Godot4 (GDScript2) Support [WIP] Godot4 (GDScript2) Support Mar 18, 2022
@elgansayer
Copy link
Contributor Author

Can someone explain this codegen thing?

@elgansayer
Copy link
Contributor Author

Most things are updated.
Need a new design for the single yield.
var data = yield() # Manually resumed

Then testing,
Things to note are the bool to strings, new json class, and coroutine calls. With some luck it may just work

@dsnopek
Copy link
Contributor

dsnopek commented Mar 28, 2022

@elgansayer Thanks for starting on this! Can you sign the CLA per the comment above? I'm not sure your code can be considered for potential merge without having signed the CLA.

@elgansayer
Copy link
Contributor Author

@elgansayer Thanks for starting on this! Can you sign the CLA per the comment above? I'm not sure your code can be considered for potential merge without having signed the CLA.

I have done? I think the app hooks are not working?
I have only to complete the yield thing but i've been too busy. It may require some proper restructuring of the code. sigh

@dsnopek
Copy link
Contributor

dsnopek commented Mar 29, 2022

I have done? I think the app hooks are not working?

My understanding of the current error message, is that the email address on your commits ([email protected]) doesn't match any of the email addresses on your GitHub account. So, perhaps you need to add that email to your GitHub under Settings -> Emails (you can have multiple emails beyond your primary)? If you already have it there, well, then I guess it is busted somehow. :-( You could try the "recheck" link in the comment up at the top?

I have only to complete the yield thing but i've been too busy. It may require some proper restructuring of the code. sigh

Ok, thanks. I haven't had a chance to review what you've already done yet, but I hope to soon!

@elgansayer
Copy link
Contributor Author

I have done? I think the app hooks are not working?

My understanding of the current error message, is that the email address on your commits ([email protected]) doesn't match any of the email addresses on your GitHub account. So, perhaps you need to add that email to your GitHub under Settings -> Emails (you can have multiple emails beyond your primary)? If you already have it there, well, then I guess it is busted somehow. :-( You could try the "recheck" link in the comment up at the top?

I have only to complete the yield thing but i've been too busy. It may require some proper restructuring of the code. sigh

Ok, thanks. I haven't had a chance to review what you've already done yet, but I hope to soon!

Ahh! Silly me. I used my work PC didn't i. oops. :D Shh, They will never know 👀

No worries. I really need to pick this back up again. I don't want it too far behind master/main branch! This last yield doesnt seem too bad. It's just an event that needs reorganising.

I would appreciate any feedback so we can get closer to a 4 branch.

@elgansayer
Copy link
Contributor Author

It may be as simple as replaying it with await _conn. Need to study it more. Clears all the errors away.

I shall start attempting to build a demo project that can use the API.

@elgansayer
Copy link
Contributor Author

elgansayer commented Apr 11, 2022

Annoyingly gdscript2 seems unable to provide the property by name, it's likely a bug! but it's a bug thats hindering keeping the script structure the same. I'll investigate it a little more before bothering gd devs.

Within deserialize
cls.get("_SCHEMA") always returns null :(
The const _SCHEMA also was not working so was changed to var. I am assuming bugs!

Any ideas anyone?

EDIT: There seems to be some discussion on properties not showing up...
godotengine/godot#43491

It could be related since _SCHEMA are no longer const. hmmmm bugger

@dsnopek dsnopek mentioned this pull request Apr 12, 2022
@dsnopek
Copy link
Contributor

dsnopek commented Apr 12, 2022

I've started poking around in your PR and attempting to get something minimally working. I've got my additions in PR #103

I suspect we may end up working on this in phases, where the 1st phase is getting something that works at all (with some workarounds for current the GDScript bugs), and then progressively making it better as we can remove the workarounds. But getting all the bugs reported early (and tracking them) will hopefully allow them to get fixed before the final Godot 4.0 releases, so we can have a nicely working client from day one.

@elgansayer
Copy link
Contributor Author

ah fantastic! I see you noticed the same issues with getting property values but actually found the bug reports too! nice work and thanx for the time dedication!

It took me a while to find those bugs as i had to slowly crawl through unknown code.

I see your workarounds too, Shame. I agree on the phrase strategy proposed.

Perhaps we should close this and focus on your draft? Mostly my changes since have been experiments with similar workarounds and trying to suss things out.

I basically look at this during my lunch hours. I will pause for now. Suggestions for todo are welcome.

@dsnopek
Copy link
Contributor

dsnopek commented Jun 1, 2022

Closing this one in favor of PR #103, which was just merged into the 'godot-4' branch

@dsnopek dsnopek closed this Jun 1, 2022
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.

3 participants