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

Update to Godot 4.0 beta9. #132

Merged
merged 1 commit into from
Dec 20, 2022
Merged

Conversation

Faless
Copy link
Contributor

@Faless Faless commented Dec 19, 2022

No description provided.

@twaritwaikar
Copy link
Contributor

twaritwaikar commented Dec 19, 2022

Thank you for spiking on this! I tried to build it on my side but I get build errors like this:

link /nologo /dll /out:demo\addons\godot-git-plugin\win64\libgit_plugin.windows.x86_64.template_debug.dll /implib:demo\addons\godot-git-plugin\win64\libgit_plugin.windows.x86_64.template_debug.lib /LIBPATH:godot-cpp\bin /LIBPATH:thirdparty\bin libgodot-cpp.windows.template_debug.x86_64.lib git2.lib ssh2.lib Advapi32.lib Winhttp.lib Rpcrt4.lib crypt32.lib OLE32.lib user32.lib godot-git-plugin\src\gdlibrary.obj godot-git-plugin\src\git_callbacks.obj godot-git-plugin\src\git_plugin.obj godot-git-plugin\src\git_wrappers.obj
LINK : fatal error LNK1181: cannot open input file 'libgodot-cpp.windows.template_debug.x86_64.lib'
scons: *** [demo\addons\godot-git-plugin\win64\libgit_plugin.windows.x86_64.template_debug.dll] Error 1181
scons: building terminated because of errors.

I don't see any indications that godot-cpp failed to build but I don't see it being generated in godot-cpp/bin

Comment on lines +27 to +28
if ARGUMENTS.get("custom_api_file", "") != "":
ARGUMENTS["custom_api_file"] = "../" + ARGUMENTS["custom_api_file"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This won't be needed after godotengine/godot-cpp#968

@twaritwaikar
Copy link
Contributor

Looks like get_branch_list() and get_remote_list() should be returning TypedArray<String>, but in the bindings, they are being shown as returning an array of Dictionaries instead of Strings

ERROR: Attempted to push_back a variable of type 'String' into a TypedArray of type 'Diction.
   at: (C:\Users\Twarit\Documents\Github\godotengine\godot\core\variant\container_type_valid)
ERROR: Condition "!_p->typed.validate(value, "push_back")" is true.
   at: Array::push_back (core\variant\array.cpp:265)
GitPlugin: Could not create remote. Error -4: remote 'origin' already exists in godot-git-pl8
ERROR: Transient parent has another exclusive child.
   at: (scene\main\window.cpp:685)
ERROR: Attempted to push_back a variable of type 'String' into a TypedArray of type 'Diction.
   at: (C:\Users\Twarit\Documents\Github\godotengine\godot\core\variant\container_type_valid)
ERROR: Condition "!_p->typed.validate(value, "push_back")" is true.
   at: Array::push_back (core\variant\array.cpp:265)
ERROR: Attempted to push_front a variable of type 'String' into a TypedArray of type 'Dictio.
   at: (C:\Users\Twarit\Documents\Github\godotengine\godot\core\variant\container_type_valid)
ERROR: Condition "!_p->typed.validate(value, "push_front")" is true.
   at: Array::push_front (core\variant\array.cpp:647)
()

@Faless
Copy link
Contributor Author

Faless commented Dec 19, 2022

@twaritwaikar sorry, I forgot to mark it as draft, this should build now, re-using the upstream godot-cpp build system as much as possible. I've tested the linux build (just running the demo, ensuring the library is loaded), and tested that windows builds correctly using mingw (msvc is untested, but should work too).

@twaritwaikar
Copy link
Contributor

Can confirm that MSVC is building properly.

I will do some adhoc feature testing then this should be great to merge.

There are a couple issues that I can reproduce from the last 4.0b build that I had tested, mainly related to the UI, so the porting work shouldn't be too much once libssl and libcrypto are linked as static libs instead of dynamic ones, but that can be handled in a different PR.

Thank you for all the work!

@Faless
Copy link
Contributor Author

Faless commented Dec 19, 2022

Looks like get_branch_list() and get_remote_list() should be returning TypedArray<String>, but in the bindings, they are being shown as returning an array of Dictionaries instead of Strings

Yeah, the bindings seem broken in Godot: https://github.com/godotengine/godot/blob/master/editor/editor_vcs_interface.h#L109-L131

Would be great if you could open a PR there.

@Faless Faless changed the title Update to Godot 4.0 beta7. Update to Godot 4.0 beta9. Dec 19, 2022
@Faless Faless marked this pull request as draft December 19, 2022 21:55
Simplify build system.
@twaritwaikar
Copy link
Contributor

twaritwaikar commented Dec 20, 2022

@Faless I am in the process of opening a PR to Godot to fix the bindings (and a couple others to fix some UI stuff).

Would you mind if I built on top of your work to get the plugin built and then a few changes to make it work nicer with Godot from a UX perspective?

@twaritwaikar
Copy link
Contributor

If yes, then this PR would be great to merge in and then I can send my changes later in a different PR

@lalomartins
Copy link

I took it for a spin here. It builds, and all functionality I was able to find works for me. (I haven't tried remote because my repo uses https and token… maybe another task for the future)

Had to poke around to figure out how to install it, though 😹 probably a good idea to set up a 3.0 wiki page before releasing it. I'll try making a PR.

Speaking of, would it be less confusing maybe if we skipped 3.0 and called it 4.0 instead?

One more thing, idk if it should be a separate ticket: the split diff panel has a glitch, line numbers are overlapping the diff text.
image

@twaritwaikar
Copy link
Contributor

twaritwaikar commented Dec 20, 2022

my repo uses https and token

It should still be possible to use an HTTPS remote with a PAT. You will need to use a remote that's pointing to an HTTPS endpoint and have an access token set here:
image

I'll try making a PR.

Feel free to draft it in an issue or a PR. I don't think Github Wiki supports PR yet but it uses the same Markdown style so we can draft things under PR descriptions.

[EDIT: The features should more or less be the same as 2.x]

maybe if we skipped 3.0 and called it 4.0 instead?

I am not completely opposed to this idea. I am expecting that we will need to release a new build with each 4.x release anyway so it would make sense to do this. @akien-mga Any thoughts here? Should we skip 3.0 and call it godot-git-plugin 4.0? We can also discuss in RC instead

the split diff panel has a glitch, line numbers are overlapping the diff text.

Yeah, I noticed that in my previous testing as well. Doesn't seem like the behaviour has changed. There are some other issues that I had noticed back then as well - godotengine/godot#62157.

@lalomartins
Copy link

my repo uses https and token

It should still be possible to use an HTTPS remote with a PAT. You will need to use a remote that's pointing to an HTTPS endpoint and have an access token set here:

Hmm, nope. I get this error: Could not lookup remote "<null>". Error -3: remote '<null>' does not exist. Maybe because mine is not on github? (It's a gitea private repository)

I'll try making a PR.

Feel free to draft it in an issue or a PR. I don't think Github Wiki supports PR yet but it uses the same Markdown style so we can draft things under PR descriptions.

Ooh yeah no wiki PRs yet right 🤔 well, I'll draft it in my fork and we can figure it out what to do with it later, once we're happy with the text. Plain old git CLI is always an option 😸

[EDIT: The features should more or less be the same as 2.x]

Yes. Install instructions are different, especially from source, and screenshots need to be updated. Also the page should tell people not to panic about not being able to load it in the plugins panel because that's not how gdextension plugins work 😹 (so they don't waste 40min on it like I did)

@twaritwaikar
Copy link
Contributor

Could not lookup remote "<null>". Error -3: remote '<null>' does not exist

This error will be fixed in Godot once this is merged - godotengine/godot#70342

That should be the only thing stopping you from pushing / pulling. If you are interested, we can talk in https://chat.godotengine.org/ since the discussion for this specific PR is pretty much done

@akien-mga
Copy link
Member

maybe if we skipped 3.0 and called it 4.0 instead?

I am not completely opposed to this idea. I am expecting that we will need to release a new build with each 4.x release anyway so it would make sense to do this. @akien-mga Any thoughts here? Should we skip 3.0 and call it godot-git-plugin 4.0? We can also discuss in RC instead

Versioning plugins that depend on versioned API is always tricky. It can make sense to reuse the major from Godot, but if the numbers stay independent beyond that you might still have confusing situations where for Godot 4.1 you need to use godot-git-plugin 4.3, etc.

@twaritwaikar
Copy link
Contributor

We have been required to make multiple plugin versions for a single Godot release so unless we add another .x to the version number, it can get confusing, as you said. Let's keep it how it is so that we are at least consistent and confusing instead of being both inconsistent and confusing later on.

@lalomartins
Copy link

That's a good point, if godotengine/godot-proposals#4369 is implemented it would mean a new major for the plugin.

@Faless Faless marked this pull request as ready for review December 20, 2022 12:12
@twaritwaikar twaritwaikar merged commit 20cb080 into godotengine:master Dec 20, 2022
@twaritwaikar
Copy link
Contributor

Thanks!

@Faless Faless deleted the spike/beta7 branch December 20, 2022 14:28
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