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

New godot compiler warnings #24

Closed
wants to merge 2 commits into from
Closed

Conversation

Mangonels
Copy link
Collaborator

CMakeLists.txt godot compiler warnings inclusion was outdated, the godot-cpp submodule was pointing to an old commit, 2 months ago a new CMakeLists.txt was applied, so if someone reset the submodules for roguelite or made a templated project based on it they could encouter cmake errors on build.

I reset all the submodules, so now they should all be up to date.

I also had to cast a float to double since it seems the angle godot::Math::lerp_angle function changed to either all double parameters or all float parameters.

Project works fine.

@Mangonels Mangonels requested a review from vorlac December 5, 2024 03:19
@vorlac
Copy link
Owner

vorlac commented Dec 6, 2024

It looks like the build is failing: https://github.com/vorlac/godot-roguelite/actions/runs/12172566104/job/33951474735#step:4:68

When it comes to stuff like this, the submodules in the project are all locked to a specific revision. This means if/when someone templated, cloned, or forked the project, they should end up with the submodules in their repo pointing to the same exact revisions (even if they're not the latest). I think it should be OK for the most part unless someone goes out of their way to change the submodule revision - but that can't/shouldn't try to be handled in this project since it would become a pain to maintain over time. If people change the submodules on their own, they should be expected to be able to fix any failures it leads to.

Either way updating the submodules to the latest version is good to do if there's new stuff to use in godot-cpp. When it comes to that it just seems like something is causing it to fail during the cmake configuration.

There's a chance it's just Github actions being dumb and not updating the submodules properly. I think I ran into something like that in the past where I had to just wipe out it's cached environment and have it build from a fresh clone. If the error it's reporting doesn't make sense to you @Mangonels, I can try that out to see if it leads to the configuration/build succeeding. Just ping me if you want me to try that out for you.

@Mangonels
Copy link
Collaborator Author

Mangonels commented Dec 6, 2024

Hey @vorlac , it doesn't make sense to me from Windows perspective (where the project can be played both as preview and standalone build, and cmake runs fine on VS), Linux however I cannot currently test, but from a theoretical standpoint it shouldn't make sense either, since the same submodule is being pointed at, and the inclusion giving errors is for a file it should be able to find.

I do have setting up a linux based environment in my pending to-do list though, so I should be able to check on this eventually, you don't need to do anything but it would be cool if you checked it out.

Updating the submodules and that particular CMakeLists.txt line is most likely going to be necessary for keeping this project updated to the last version of Godot in the future. Also the godot-cpp CMakeLists.txt is undergoing changes (I think you were even envolved in them? haha) so on further updates things will definetly break.

I'll be doing updates indefinetly since I'm using this project as a base for my own and I'd like to see the setup still working on each latest godot version, while also sharing it with others.

@vorlac
Copy link
Owner

vorlac commented Dec 6, 2024

Sounds good. Yeah, I agree, it'll be good to keep things up to date.

The submodules are just part of the git repo so Linux/windows won't make a difference.

You can always use the GitHub actions to test the changes on Linux. If you push a change to the same branch you used for this PR, the bot will invoke a new build for each push. Feel free to spam it as much as you want. I think you probably also have permissions to clear the GH actions history if that starts to get messy, it doesn't matter to me though.

Lmk if you aren't able to get it working when you revisit it. I should have time this weekend to take a look at things with you to see if we can figure out what's happening.

When I get a chance I'll also wipe out the GH actions local cache on my server to see if a clean env will end up building fine.

@vorlac
Copy link
Owner

vorlac commented Dec 25, 2024

fyi - i'm going to close this since i noticed the build was broken using the submodules at the revisions they were both at. I just updated them to the 4.3 branch and updated the cmake compiler flags include() call in the main cmakelists.txt.

@vorlac vorlac closed this Jan 5, 2025
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