Skip to content

Godot integration to main#7

Draft
Sythelux wants to merge 7 commits into
runevision:mainfrom
Sythelux:main
Draft

Godot integration to main#7
Sythelux wants to merge 7 commits into
runevision:mainfrom
Sythelux:main

Conversation

@Sythelux
Copy link
Copy Markdown

This is not finished, but how I would imagine the integration.

in Src:

  • I tagged all files in the Unity folder with the compiler flag #if UNITY_2019_4_OR_NEWER
  • I added a Godot folder and added the implementation for the Godot side into it and also added a #if Godot4
  • this should not compile
  • Godot is missing a couple of things like the file based player prefs, which I will add as file based as well, because Godot already offers a "user://" folder shortcut, but I need to port a bit of code.

I added the exclusion of the Godot folder to the upm deploy script so that it doesn't move unnecessary files.

I added 2 new python scripts, one to deploy to the godot_project branch, which has a fully working godot project with Samples, but which can also be used to export an addon zip later for the Assetshop (I hope)

The second script is only the addon folder flat (with plugin.cfg in root), which could also be used for Assetshop export, but can also be used by other godot project to add it as git submodule.

I leave this as draft until I'm happy with the implementation, but feel free to comment on the things I mentioned in here.

@runevision
Copy link
Copy Markdown
Owner

Thanks for your work on the Godot integration!

My Godot knowledge is limited so I'll focus on aspects of the PR that I can meaningfully comment on (some of them a bit superficial perhaps).

I tagged all files in the Unity folder with the compiler flag #if UNITY_2019_4_OR_NEWER

Looks sensible.

  • I added a Godot folder and added the implementation for the Godot side into it and also added a #if Godot4
  • this should not compile

Do you mean that this would cause it to not be compiled in Unity (as intended)? Just checking since "this should not compile" being a separate bullet point made me unsure.

Godot is missing a couple of things like the file based player prefs, which I will add as file based as well, because Godot already offers a "user://" folder shortcut, but I need to port a bit of code.

I think this part is perhaps not urgent, given there is no sample scene that shows the SaveState functionality, or even documentation on how to use it. I've been meaning to add that when I had time. Until then, I'm not sure how you would test/confirm that the Godot integration of it works correctly?

I added the exclusion of the Godot folder to the upm deploy script so that it doesn't move unnecessary files.

Thanks!

I added 2 new python scripts, one to deploy to the godot_project branch, which has a fully working godot project with Samples, but which can also be used to export an addon zip later for the Assetshop (I hope)

The second script is only the addon folder flat (with plugin.cfg in root), which could also be used for Assetshop export, but can also be used by other godot project to add it as git submodule.

I don't understand this well. The two new python files have the same comment at the top. Is this intentional or an oversight? If they do something different, I don't think they can share the same branch?

I leave this as draft until I'm happy with the implementation, but feel free to comment on the things I mentioned in here.

Sounds good! Some additional comments:

For the GenerationSource you added your name to the copyright. We should ensure proper copyright notices for all the files you added (at least C# files). I would suggest these two forms, the first for files that you based on files I wrote, and the second for files you wrote from scratch:

/*
 * Godot adaptation copyright (c) 2024 Sythelux Rikd
 *
 * Based on:
 * LayerProcGen copyright (c) 2024 Rune Skovbo Johansen
 *
 * This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at https://mozilla.org/MPL/2.0/.
 */

/*
 * LayerProcGen Godot integration copyright (c) 2024 Sythelux Rikd
 *
 * This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at https://mozilla.org/MPL/2.0/.
 */

Some notes on formatting:

We should ensure files have a newline at the end (e.g. last line of a file should be empty) as it seems to be best practice and avoid confusion in diff tools. Some of your edits to existing files removed newlines (visible in the GitHub diff as red circles with a minus inside.
https://stackoverflow.com/questions/77201426/what-does-a-red-icon-with-a-hyphen-mean-in-a-github-pr

It seems Godot docs officially recommend spaces over tabs, so I understand why all the Godot-specific C# files use spaces unlike the rest of the codebase. Ah well.

Keep up the good work!

@Sythelux
Copy link
Copy Markdown
Author

Do you mean that this would cause it to not be compiled in Unity (as intended)? Just checking since "this should not compile" being a separate bullet point made me unsure.

yes I meant it as the #if Godot part shouldn't compile in Unity so I it should have the analogous compiler flags

I think this part is perhaps not urgent, given there is no sample scene that shows the SaveState...

I don't know either I usually would implement it, but mark it as untested with a comment. In this case I might wait for you with an example and leave the implementation open until then.

I don't understand this well. The two new python files have the same comment at the top...

this is a copy paste error, one was ../godot_addon and one is ../godot_project
I also have 2 draft branches for each:

For the GenerationSource you added your name to the copyright...

I just wasn't sure about the etiquette on that. I will change accordingly.

We should ensure files have a newline...

I will keep that in mind. The "concat file" example seems logical.

and I can switch to tabs for you. I don't mind that my Texteditor does the conversion automatically as I type and this is a mixed Project so you can have the last say in it.

Keep up the good work!

Thank you ~

Sythelux added 4 commits May 29, 2024 20:33
Added only halfway working _Get and _Set in editor.

Added extra line at the end in some files
new: height maps are now being rendered
chg: all files should have a new line at the end
chg: license header was wrong
Copy link
Copy Markdown
Author

@Sythelux Sythelux left a comment

Choose a reason for hiding this comment

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

This is how it looks currently. height maps work. The memory allocation is a mess and nothing else works, but well still steady progress.

MountainGen.mp4

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