func-godot / func_godot_docs

Documentation for the FuncGodot plugin.
MIT License
4 stars 4 forks source link

Runtime map building + funcGodotMap function reference #8

Open RisingThumb opened 2 months ago

RisingThumb commented 2 months ago

This should resolve #4

@sinewavey I'd be interested to hear your insight on this since I know you have a lot more runtime map building experience than I do, is there anything obvious I'm missing here. @RhapsodyInGeek what are your thoughts on this, about what should and shouldn't be included here?

There is a new page under tips and tricks titled runtime map building. There is also a new table added to the funcGodotMap class talking about the methods. I'm unsure about how best to list the parameters for the unwrap_uv2 method, since it's an optional parameter.

Additionally under the runtime map building page, I've noted that it's left to the user to bake what they want. I'm unsure if what we should be saying about the unwrap UV2 method here. idk how useful this is currently with this issue still existing https://github.com/godotengine/godot-proposals/issues/8656#issuecomment-2052287278

Should we be documenting the import limitation for exported files? Since it's a pitfall that doesn't have an obvious solution and affects current stable build and the current 4.3 build(both requiring either of the other 2 import options selected instead of QuakeMap) I'm in favour of keeping this.

I have also documented the tip about using command line argument to pass map files as a launch option for Trenchbroom(Similar to Sinewave's point about this in the Discord for radiant). It's one of the cool not-so-obvious benefits of runtime map building

You can preview these changes here: https://risingthumb.github.io/func_godot_docs/FuncGodot%20Manual/FuncGodot%20Manual.html

RhapsodyInGeek commented 2 months ago

There is also a new table added to the funcGodotMap class talking about the methods. I'm unsure about how best to list the parameters for the unwrap_uv2 method, since it's an optional parameter.

You should write the function name exactly as it is defined in the GDScript file: unwrap_uv2(node: Node = null). Additionally, we should specify the return type as well, even if the return type is just void.

To maintain consistency across the plugin and the documentation, use the official description comments found in the script files rather than creating your own. So for unwrap_uv2:

Recursively unwrap UV2s for node and its children, in preparation for baked lighting. And for verify_and_build(): _Verify that FuncGodot is functioning and that [member map_file] exists. If so, build the map. If not, signal [signal buildfailed]

Functions should be listed in the order that they appear in the script files, just like signals and properties do.

If a description is inaccurate, definitely report it on the plugin issues.

Also, this is on me in a way since I'm pretty sure I added the signals in the reference, but we should order things the same way the Godot docs do, I think. So it should be Properties first, Methods second, Signals third. Don't worry about changing that though, I'll go through all the reference pages and fix it all at once.

There is a new page under tips and tricks titled runtime map building.

I think we shouldn't jump the gun on it quite yet. We first need a project set up for minimal steps to runtime building so that we can make any tutorial on it follow that. That means a project with nothing but an unmodified func_godot, a simple map, and a scene with a single custom script to perform runtime building on _ready() or on key press. When we have that, we can then document the steps to recreate that and write the Runtime Building Tips page based on that project, so that we know exactly what is needed.

Should we be documenting the import limitation for exported files? Since it's a pitfall that doesn't have an obvious solution and affects current stable build and the current 4.3 build(both requiring either of the other 2 import options selected instead of QuakeMap) I'm in favour of keeping this.

I think it's worth documenting. I admittedly don't think it'll really be an issue for most users since runtime building is more likely going to be used for custom maps which will be external files anyway, and the problem is caused by Godot saving packed .map files as .tres. That said, it's an opaque issue that can affect devs trying to do procedural map building with map files packaged with the PCKs on export, so to save those few the hassle of figuring it out we should document how the behavior of Godot's resource packing can affect runtime building of internal map files.

A couple other notes on the Runtime Building page:

I appreciate the efforts and enthusiasm for trying to improve the docs though. Usually feels like a sorely neglected aspect of most tools out there.

RisingThumb commented 2 months ago

Thanks for the comprehensive review!

but we should order things the same way the Godot docs do, I think. So it should be Properties first, Methods second, Signals third. Don't worry about changing that though, I'll go through all the reference pages and fix it all at once.

I've raised a separate issue for this and other "across the entire docs" issues can be addressed in one whole lot.

I think we shouldn't jump the gun on it quite yet. We first need a project set up for minimal steps to runtime building so that we can make any tutorial on it follow that. That means a project with nothing but an unmodified func_godot, a simple map, and a scene with a single custom script to perform runtime building on _ready() or on key press. When we have that, we can then document the steps to recreate that and write the Runtime Building Tips page based on that project, so that we know exactly what is needed.

I think this would be best added to the the func_godot_example_basic repo. I can put together a basic example for this when I get the time. Or should this be in the test repo? I think it's more demonstrative than it is testing. Do you think we should be tutorialising it? I think making an example, pointing to it in the example repo and just making it known that you can do this and the common pitfalls would be sufficient imo.

I'll take a look at addressing the other simpler parts of this review when I get more free time to look at it later this week

RhapsodyInGeek commented 2 months ago

Added a runtime building example project: https://github.com/func-godot/func_godot_runtime_building

sinewavey commented 2 months ago

Sorry I completely missed this in the haze of traveling around the US and returning back home.

I think you hit the primary points of it pretty well. Regarding the last tip line - I actually just wrote a docs page on that without realizing this existed 😅 I think it'd be wise to consider uniting the two.

Lightmap building is something that you can't currently do in runtime or exported builds; only the editor. Coincidentally, I've been looking into writing a q3map2-like "Compiler" that will still allow users to save a .map, run a compiler program, and get a .tscn with the baked lightmap data and scene ready to play, much like the process goes in Quake.

With what Tim has mentioned regarding VoxelGI, it makes me think: why can't any node like that (define an area to bake) work this way? If particleSDF fields can be baked outside of the editor, it'd be another useful tool to expose. And in that case, I think the two sections almost deserve a different approach with discussing those steps proper, with a breakdown of runtime vs in editor for each.

That said, good lord is this a process to figure out so far as it's heavily intertwined with the Godot UI.

RisingThumb commented 2 months ago

Regarding the last tip line - I actually just wrote a docs page on that without realizing this existed 😅

Personally, I think they should be separated. One is about launching within an editor(which is most useful with runtime map building, there probably exists other reasons to use it but idk), and the other is about runtime map building. I'll remove this tip from the page since your PR addresses it

Lightmap building is something that you can't currently do in runtime or exported builds; only the editor. Coincidentally, I've been looking into writing a q3map2-like "Compiler" that will still allow users to save a .map, run a compiler program, and get a .tscn with the baked lightmap data and scene ready to play, much like the process goes in Quake.

Imo, all that needs to be done is for the upstream work to be done on exposing lightmap baking in the node, and adding a --compile-lightmaps CLI arg. Once that's done, you compile it at runtime, to spit out a .lmbake file that's used every other time when loading the map.

sinewavey commented 2 months ago

Imo, all that needs to be done is for the upstream work to be done on exposing lightmap baking in the node, and adding a --compile-lightmaps CLI arg. Once that's done, you compile it at runtime, to spit out a .lmbake file that's used every other time when loading the map.

In theory it's pretty simple. In practice... it's a lot more complicated, from a cursory glimpse at how the LightmapGI stuff even works in the editor.

Exposing it to the user: yes. Actually getting this feature: that's a doozy.

RisingThumb commented 2 months ago

To maintain consistency across the plugin and the documentation, use the official description comments found in the script files rather than creating your own. So for unwrap_uv2: Recursively unwrap UV2s for node and its children, in preparation for baked lighting. And for verify_and_build(): Verify that FuncGodot is functioning and that [member map_file] exists. If so, build the map. If not, signal [signal build_failed]

I've made this change. For the parts using [code][/code] and [member] and [signal] I've changed these to just be <code></code> tags. I believe these are some form of markdown docs comment, but idk. Happy to retain that, but I think it looks bad like that

Personally, I think they should be separated. One is about launching within an editor(which is most useful with runtime map building, there probably exists other reasons to use it but idk), and the other is about runtime map building. I'll remove this tip from the page since your PR addresses it

In addition, I've made this change. @sinewavey if this gets merged in before your PR, it might be worthwhile adding a note and a link forward under this page about launching straight from the editor being one of the benefits of this.

I think we shouldn't jump the gun on it quite yet. We first need a project set up for minimal steps to runtime building so that we can make any tutorial on it follow that. That means a project with nothing but an unmodified func_godot, a simple map, and a scene with a single custom script to perform runtime building on _ready() or on key press. When we have that, we can then document the steps to recreate that and write the Runtime Building Tips page based on that project, so that we know exactly what is needed.

I have linked forward to the example repository you made and I've copied the script over verbatim.

Also, this is on me in a way since I'm pretty sure I added the signals in the reference, but we should order things the same way the Godot docs do, I think. So it should be Properties first, Methods second, Signals third. Don't worry about changing that though, I'll go through all the reference pages and fix it all at once.

This has been left to be addressed in #9 . Any further thoughts are welcome, happy to make other small changes! 👍

sinewavey commented 2 months ago

In addition, I've made this change. @sinewavey if this gets merged in before your PR, it might be worthwhile adding a note and a link forward under this page about launching straight from the editor being one of the benefits of this.

Yep, if the other docs are updated, as would my proposed PR page until it's live. Part of submitting PRs - includes fixing other ones if needed.

RhapsodyInGeek commented 2 months ago

Personally, I think they should be separated. One is about launching within an editor(which is most useful with runtime map building, there probably exists other reasons to use it but idk), and the other is about runtime map building. I'll remove this tip from the page since your PR addresses it

They should be together. Launching from the editor is made possible through runtime / @tool scripted building. Runtime Build page would best be:


Runtime Building

We should also check if JACK is capable of it, too.

RisingThumb commented 2 months ago

They should be together. Launching from the editor is made possible through runtime / @tool scripted building. Runtime Build page would best be:

I've added this back in the previous minimal form. I've addressed the structure changes too. Thanks for the quick reviews btw! 👍

@sinewavey would it be possible for #10 to be reworked slightly to go in the section where I've currently got the brief text about using command lines to build from map editors?

sinewavey commented 2 months ago

Easily. The language and wording of it should practically be ready to copy/paste in to your PR if you like.

RisingThumb commented 2 months ago

Easily. The language and wording of it should practically be ready to copy/paste in to your PR if you like.

Up to you, personally, I'd prefer separate PRs, but I'm happy to combine them both into this PR

RisingThumb commented 1 month ago

@sinewavey bumping in case this has been missed, If there's no preferences I'd like to go forward with this PR so this information is available instead of hidden away

sinewavey commented 1 month ago

@sinewavey bumping in case this has been missed, If there's no preferences I'd like to go forward with this PR so this information is available instead of hidden away

Absolutely. I'll go ahead and close mine.

RisingThumb commented 1 month ago

@sinewavey bumping in case this has been missed, If there's no preferences I'd like to go forward with this PR so this information is available instead of hidden away

Absolutely. I'll go ahead and close mine.

I'll look at adding the stuff from your PR into this when I get some time later this week, thanks! 👍

RisingThumb commented 1 month ago

I've addressed all outstanding PR comments. I've also added Sinewavey's work from his branch on top of this. I've left some conversations unresolved to keep them open for further discussion if needed

RisingThumb commented 1 month ago

btw @RhapsodyInGeek if it's possible, can we set up squash merging for this project(and if it's already set up, my bad)?

https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests

When this PR is merged, it'll be 8(probably more) individual commits that aren't valuable individually

RhapsodyInGeek commented 1 month ago

Looks like it's already on.

RisingThumb commented 1 month ago

I've resolved the last few comments I had on this PR about the screenshots and slightly tweaked some of the wording to move the godot cli arguments stuff into the pre-editor specific text.

I've also gone ahead and resolved most of the conversations since I had addressed them and they were stale. Feel free to reopen any that need further discussion.

Let me know if any more changes need to be made, thanks for all the reviews! 👍

RisingThumb commented 1 month ago

Also can someone confirm whether all instances of the word "Radiant" should be replaced with the word "NetRadiant"? I'm under the impression there's multiple radiant editors and that this is specific to NetRadiant, and not Radiant, but as I'm not very familiar with this idk

RhapsodyInGeek commented 1 month ago

I'm under the impression there's multiple radiant editors

There are more Radiant forks out there than there are grains of sand in the Sahara.

I'll give it a look later this afternoon.

EDIT: I didn't wait until later this afternoon.

sinewavey commented 1 month ago

Also can someone confirm whether all instances of the word "Radiant" should be replaced with the word "NetRadiant"? I'm under the impression there's multiple radiant editors and that this is specific to NetRadiant, and not Radiant, but as I'm not very familiar with this idk

As Tim mentioned there's many forks over time of Radiant. I think this is a feature that it's capable of doing since at least GTKRadiant. I'm of the opinion probably nobody uses anything older than that at this point, or maybe a custom fork of Radiant, but again, with such a limited audience...

RisingThumb commented 1 month ago

I think this is a feature that it's capable of doing since at least GTKRadiant. I'm of the opinion probably nobody uses anything older than that at this point, or maybe a custom fork of Radiant, but again, with such a limited audience...

I'll leave this as is then

RisingThumb commented 1 month ago

I've addressed the PR comments about using run_map as the command line argument and updating the screenshots accordingly.

Going into the realm of multiple build choices and etc is relevant to the map editor, but is documented better by the editor(we don't want to become a tutorial for how to use the editor, but rather how to integrate with it). @sinewavey is there a link to the NetRadiant wiki or documentation that would be appropriate for this?

Additionally the tool script stuff and the additional build option, in the interest of keeping things as simple as possible, I've omitted this. Would be happy to update the screenshot here to use the godot game engine instead of opening an exported build, but I'm not too fussed. Might even be better as-is as it means there's an example of both present.

Additionally snipped the other 2 images for the trenchbroom configuration. The first, the empty parameter page was superfluous considering the 3rd image we had, and the 2nd is self-explanatory in the editor. Also because I don't have updated images for the 2nd one(I can add it back if it's not deemed superfluous, personally, I think it's pretty obvious from the text and the UI of that page).

sinewavey commented 1 month ago

Leaving them here is totally ok with me. I was just exploring for my own project and figured I'd take pictures along the way just in case.

As far as NRC documentation/wiki. I have bad news. It like doesn't really exist and is some sparse files amongst the program.

RisingThumb commented 1 month ago

As far as NRC documentation/wiki. I have bad news. It like doesn't really exist and is some sparse files amongst the program.

Here be dragons! 🐉

RhapsodyInGeek commented 1 month ago

image We should replace the TB image again, but this time with this because `${MAP_FULL_NAME} is a better option (it includes the .map extension).

RisingThumb commented 1 month ago

image We should replace the TB image again, but this time with this because `${MAP_FULL_NAME} is a better option (it includes the .map extension).

See this comment: https://github.com/func-godot/func_godot_docs/pull/8#discussion_r1605905998 EDIT: This also means this image needs updating to reflect the separator requiring a -- run_map and not being --run_map

RisingThumb commented 1 month ago

Also an additional comment: I'm of the opinion we should probably call this PR good at some point. Aside from fixing the TrenchBroom image one more time to be perfect, I think the remaining changes are mostly for the NetRadiant Custom editor which I don't use and I'm not the right person for writing/fixing/testing that part of the documentation(and don't want to stall improvements to that part of the docs)

sinewavey commented 1 month ago

Also an additional comment: I'm of the opinion we should probably call this PR good at some point. Aside from fixing the TrenchBroom image one more time to be perfect, I think the remaining changes are mostly for the NetRadiant Custom editor which I don't use and I'm not the right person for writing/fixing/testing that part of the documentation(and don't want to stall improvements to that part of the docs)

I'm pretty sure I can just write commits addressing NRC concerns on your fork, then we can close it, if you want/can let me. Pretty sure GH should allow this easily

RisingThumb commented 1 month ago

I'm pretty sure I can just write commits addressing NRC concerns on your fork, then we can close it, if you want/can let me. Pretty sure GH should allow this easily

That'd also be a good approach. Let me know if there's anything I need to set up for that ❤

RhapsodyInGeek commented 1 month ago

Also an additional comment: I'm of the opinion we should probably call this PR good at some point.

I am not of the opinion to be honest, since I just found another issue with the TrenchBroom tutorial after some more testing. You're correct that we can't get ${MAP_FULL_NAME}, but we also can't get ${MAP_DIR_PATH} for the launch parameters. If you have your maps in any folder other than the root folder, your game won't find the map. All ${MAP_BASE_NAME} returns is the map name itself, not the path leading to it. So more properly the command line argument would be: -- run_map::<path_to_map_dir>/${MAP_BASE_NAME}.map

I know it's annoying to have to do this big back n forth and all of these "perfectionism" changes, but I want to make sure everything is right the first time and to try to maintain a standard tone, keep it in on par with the rest of the manual, just like any other major addition to the plugin goes through a rigorous process.

I'm currently updating the Runtime Example Project as well, and including an example command_line_building.tscn.

I'd like to make sure we get the NRC part right, too. I couldn't tell how you were supposed to set it up with the instructions. The order of "check out the setup" and "edit the default_build_modes.xml" felt backwards, because it seemed like you were saying to edit the XML in order to get the Setup, because there was no description of how to set the parameters before that.

RhapsodyInGeek commented 1 month ago

New TrenchBroom example image: image

RhapsodyInGeek commented 1 month ago

New command line building example:

<pre><code>extends Node

@onready var func_godot_map := $FuncGodotMap as FuncGodotMap

func _ready():
    # We want to loop through each command line argument until we find 
    # our "run_map" argument. This argument will be split using :: as a delimiter,
    # giving us a two element array with the second element being our map's path.
    for cmdline_arg in OS.get_cmdline_user_args():
        if cmdline_arg.contains("run_map::"):
            var arg_arr := cmdline_arg.split("::")

            # Safety check to make sure we didn't mess up our command line arguments.
            # We should have a String array with [argument command, argument value].
            if arg_arr.size() != 2:
                return

            # Apply the argument value to our map node and build!
            func_godot_map.local_map_file = arg_arr[1]
            func_godot_map.verify_and_build()</code></pre>
sinewavey commented 1 month ago

I know it's annoying to have to do this big back n forth and all of these "perfectionism" changes, but I want to make sure everything is right the first time and to try to maintain a standard tone, keep it in on par with the rest of the manual, just like any other major addition to the plugin goes through a rigorous process.

personally I think this is pretty normal given there isn't any standardized guidelines and this helps define them more, such as what we're trying to indicate with code comments.

I'd like to make sure we get the NRC part right, too. I couldn't tell how you were supposed to set it up with the instructions. The order of "check out the setup" and "edit the default_build_modes.xml" felt backwards, because it seemed like you were saying to edit the XML in order to get the Setup, because there was no description of how to set the parameters before that.

Given your feedback I'm going to update this as well. I mentioned the menu first to give context to the commands that one then seems to have to write - so you have to know what's available before you can write anything. It needs a rework entirely to explain in a more natural way, and just more information in general rather than my 5 second scratches.

RisingThumb commented 1 month ago

Also an additional comment: I'm of the opinion we should probably call this PR good at some point.

I am not of the opinion to be honest, since I just found another issue with the TrenchBroom tutorial after some more testing. You're correct that we can't get ${MAP_FULL_NAME}, but we also can't get ${MAP_DIR_PATH} for the launch parameters. If you have your maps in any folder other than the root folder, your game won't find the map. All ${MAP_BASE_NAME} returns is the map name itself, not the path leading to it. So more properly the command line argument would be: -- run_map::<path_to_map_dir>/${MAP_BASE_NAME}.map

${MAP_DIR_PATH}

This seems like a TrenchBroom limitation. GAME_DIR_PATH exists, but doesn't quite serve the same purpose. The trenchbroom documentation for what variables are available for engine launching does just list 3 variables sadly. We already documented that you can use these variables.

I'm not sold that it'll even be a pitfall, but I'll add a note about this anyway

New command line building example:

$Label.text = arg_arr[1] I don't think this line should be in the docs? If we're just purely copying from the example project, I think it'd be better to link directly to the example instead of copying it over, because copying it means you need to update it in 2 different places.

RhapsodyInGeek commented 1 month ago

This seems like a TrenchBroom limitation. GAME_DIR_PATH exists, but doesn't quite serve the same purpose. The trenchbroom documentation for what variables are available for engine launching does just list 3 variables sadly. We already documented that you can use these variables.

Do you know what GAME_DIR_PATH does? I can test it for sure, but if you already know what it returns that's helpful too. I know that of the other 3 variables, 2 of them are apparently explicitly for compilers and can't be used as launch parameters because they for some reason can't be converted to strings.

$Label.text = arg_arr[1] I don't think this line should be in the docs?

Oops, yeah, sorry, that was me debugging the previous issue with ${MAP_DIR_PATH} and ${MAP_FULL_NAME}. That can be omitted. But also a good case for why we should keep taking things slow with this, so we can get it correct out of the gate.

If we're just purely copying from the example project, I think it'd be better to link directly to the example instead of copying it over, because copying it means you need to update it in 2 different places.

While it does mean we'd need to update it in two different places, it's not that big of a deal in this case as the examples are simple enough they won't actually need to be changed unless Godot suddenly changes how command line arguments are read or we suddenly have the ability to do UV2 unwraps in exported projects. We'll have to update the tutorial if/when runtime lightmap baking comes in regardless, but when that comes in it's just the one-time update to the two locations at that point. The FuncGodot side of things won't be changing, so I think this is a non-issue.

RisingThumb commented 1 month ago

Do you know what GAME_DIR_PATH does? I can test it for sure, but if you already know what it returns that's helpful too. I know that of the other 3 variables, 2 of them are apparently explicitly for compilers and can't be used as launch parameters because they for some reason can't be converted to strings.

image

GAME_DIR_PATH maps to the directory set in your game preferences. Above is an example of that path. I have tested this btw, and it does map to that as I expect

I was thinking about this, you can probably use the Run > Compile Map option as another way of compiling/launching maps, but I haven't dug into it 🤔

sinewavey commented 1 month ago

See here: https://github.com/RisingThumb/func_godot_docs/pull/1

RisingThumb commented 1 month ago

Merged it @sinewavey

RhapsodyInGeek commented 1 month ago

I was thinking about this, you can probably use the Run > Compile Map option as another way of compiling/launching maps, but I haven't dug into it 🤔

Might be worth looking into! A good reliable method to automatically grab the map's global folder location would be useful. I'd do it myself, but I'm currently trying to solve https://github.com/func-godot/func_godot_plugin/issues/15

sinewavey commented 3 weeks ago

With this PR the NRC section should probably mention the adjustments

RisingThumb commented 3 weeks ago

Might be worth looking into! A good reliable method to automatically grab the map's global folder location would be useful. I'd do it myself, but I'm currently trying to solve func-godot/func_godot_plugin#15

This works. You could also probably define alternative profiles for when you want to do any preprocessing for the map(lightmap baking, navmesh baking, etc to save those). See the screenshot below. The tool path is just the path to wherever godot.exe lives. It doesn't even necessarily have to be godot.exe if you want to use a different tool beforehand to prepare the level.

image

Working directory is only necessary if you're doing things requiring relative paths. You could just as easily set the working directory to ${GAME_DIR_PATH} and then do --path ./ instead. Also yes, the parameter is the old parameter name because I haven't updated that in my project.

To be honest I think this is even simpler to explain than the launch engine solution. I'll do a rewrite of the currently written trenchbroom stuff to do it this way instead.

On the topic of different tools, I'm not very involved in the Quake space(beyond lurking in their mapping discord), but there's probably custom tools that effectively do the BSP step to remove faces out of bounds and covered. I can imagine a setup where you use func_godot for the point/model entities, and process the brushes in a different tool and then import it into Godot as a model. That's just an idea I have since one of the pain points I have with func_godot is the manual process of selecting all faces to be skip(even though it's intentionally design this way, it's a lot of extra control, and a lot of extra work as the cost for that control).

If you have any other ideas about those custom tools, I'd be interested to hear them. I think this would be a useful note to link to some example(common Quake .map processing tools) and some example usecases for them.

EDIT: As an example... ericw tools to create the bsp -> bsp2obj(https://github.com/fzwoch/bsp2obj) -> godot as a model. You'd have a point model at origin representing the map, as exported from the bsp, and you'd just avoid brush entities when making your fgd.