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

Improve Main Menu options and fix misc bugs #166

Merged
merged 4 commits into from
Mar 26, 2023

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Mar 1, 2023

Depends on #163
Fixes most issues in #162
Fixes: #164

  1. Turns on looping for the flying forklift animation
  2. Fixes scaling of the crosshair
  3. Switches to using physical keyboard layouts
  4. Replaces resolution settings with 3D_scaling
  5. Fixes settings menu (same as [4.0] Fix the settings menu #165)
  6. Updates settings menu settings to work with
  7. Replaces "Root" of .import files with "Node3D" instead of "Spatial"
  8. Uses buttons throughout the main menu so the buttons appear consistent
  9. Add GI Type menu options (SDFGI, VoxelGI, or LightmapGI + Reflection Probes)
  10. Removes the refraction setting from glass tiles (scale was 0 anyway so it wasn't actually doing anything) This dramatically improves performance on low-end devices
  11. Fixes robot aim

This PR gets the demo much closer to the functionality we want for 4.0.

@clayjohn clayjohn added this to the 4.0 milestone Mar 1, 2023
@clayjohn clayjohn changed the title Menu Improve Main Menu options and fix misc bugs Mar 2, 2023
main/main.gd Outdated
Comment on lines 5 to 6
if DisplayServer.get_name() == "headless":
Engine.max_fps = 60
Copy link
Member

@Calinou Calinou Mar 2, 2023

Choose a reason for hiding this comment

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

Not related to this PR, but this trick should probably be documented in the High-level multiplayer documentation. (In general, we need better documentation on controlling tickrate, choosing a tickrate, etc.)

project.godot Show resolved Hide resolved
level/level.gd Outdated Show resolved Hide resolved
Comment on lines +80 to +81
# LightmapGI nodes override SDFGI (even when hidden)
# so we need to free the LightmapGI node if it exists
Copy link
Member

@Calinou Calinou Mar 2, 2023

Choose a reason for hiding this comment

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

This sounds like an engine bug (same for VoxelGI-LightmapGI interaction). I'd expect hiding the LightmapGI node to have the other GI technique take over automatically, so that providing a fallback is easier.

I can create an issue with a MRP if you want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is an engine bug (or that the very least a poor interaction that should be improved)

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works.

There's a change that could be made to the SDFGI high quality setting before merging though (see my review comment).

LightmapGI seems very dark in the outside area, but I'm not sure how to fix this since there's no longer an energy multiplier available (unlike BakedLightmap in 3.x).

@clayjohn
Copy link
Member Author

clayjohn commented Mar 3, 2023

LightmapGI seems very dark in the outside area, but I'm not sure how to fix this since there's no longer an energy multiplier available (unlike BakedLightmap in 3.x).

Heh, I noticed the same thing. With SDFGI and with VoxelGI we boost the energy by 2.2 times to account for how dark the scene is. Its definitely something to keep in mind as we revisit and improve LightmapGI

@Calinou
Copy link
Member

Calinou commented Mar 6, 2023

I've merged #163. This PR needs a rebase before this can be merged.

@clayjohn
Copy link
Member Author

clayjohn commented Mar 9, 2023

@Calinou Rebased and fixed a couple more bugs. This should be good to merge!

The only thing of concern is that the size of the lightmap file is 68 mb which is larger than Github recommends. On pushing the branch I got a warning that 50mb is the maximum recommended file size.

@Calinou
Copy link
Member

Calinou commented Mar 11, 2023

The only thing of concern is that the size of the lightmap file is 68 mb which is larger than Github recommends. On pushing the branch I got a warning that 50mb is the maximum recommended file size.

I've baked high-quality lightmaps (Ultra quality, 16 bounces) which are only 20 MB in size, presumably as they're less noisy and compress better: level_lightmaps.zip

However, two floor structures in the inner room use a fully cyan color with this lightmap for some reason.

PS: Did you have Directional enabled when baking lightmaps? I had it disabled when baking here. We should write out the instructions for rebaking lightmaps somewhere, as this may be needed at some point in the future.

@clayjohn
Copy link
Member Author

The only thing of concern is that the size of the lightmap file is 68 mb which is larger than Github recommends. On pushing the branch I got a warning that 50mb is the maximum recommended file size.

I've baked high-quality lightmaps (Ultra quality, 16 bounces) which are only 20 MB in size, presumably as they're less noisy and compress better: level_lightmaps.zip

However, two floor structures in the inner room use a fully cyan color with this lightmap for some reason.

PS: Did you have Directional enabled when baking lightmaps? I had it disabled when baking here. We should write out the instructions for rebaking lightmaps somewhere, as this may be needed at some point in the future.

I didn't enable Directional as I found the added size wasn't worth it.

It looks like your bake didn't include many assets. Here is what I get when I use it.

Screenshot from 2023-03-11 12-53-05

And here is how it looks without any GI

Screenshot from 2023-03-11 12-53-12

It looks like the pillars don't have lightmaps at all. And the things that do have lightmaps have turned completely black.
When I captured lightmaps originally I had a similar issue and I had to reimport all the assets with static GI + lightmaps enabled so they had UV2s.

@Calinou
Copy link
Member

Calinou commented Mar 13, 2023

When I captured lightmaps originally I had a similar issue and I had to reimport all the assets with static GI + lightmaps enabled so they had UV2s.

I've tried reimporting all assets1 this way to no avail. It seems the objects that get included every bake are picked almost randomly, so I think we should just merge this until we can figure out how to bake high-quality lightmaps properly.

Footnotes

  1. I kept the existing bake modes, since the light shafts/particle/player models shouldn't be included in the bake.

@Calinou Calinou merged commit bd457e3 into godotengine:4.0-dev Mar 26, 2023
@clayjohn clayjohn deleted the menu branch April 3, 2023 17:31
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.

2 participants