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

Fix LightmapGI not working with MultiMeshes #97755

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tdaven
Copy link
Contributor

@tdaven tdaven commented Oct 3, 2024

Resolves #56027

This commit adds support for calling get_meshes on the multimesh_instance_3d. The order of the transform and mesh differs from get_bake_meshes from gridmap so swap the other so they match. This appears to fix the issue.

This fixes uses the attempt from #56027 (comment) but fixes the ordering in the return so the bake works correctly.

@tdaven tdaven requested a review from a team as a code owner October 3, 2024 04:16
@tdaven tdaven force-pushed the fix-56027 branch 2 times, most recently from 9e5c9a5 to e754253 Compare October 3, 2024 04:23
@tdaven
Copy link
Contributor Author

tdaven commented Oct 3, 2024

An easy alternative way to fix this would be to add a get_bake_mesh that returns the result in the correct order. I'm not sure what is best. It would duplicate a lot of get_meshes, which was the primary reason I just added an arg with a default value that preserves the old behavior but allows swapping for this use.

@AThousandShips AThousandShips added bug topic:rendering topic:3d cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Oct 3, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Oct 3, 2024
@tdaven
Copy link
Contributor Author

tdaven commented Oct 3, 2024

It wasn't clear to me if we want to be doing more of the work related to UV2 that is done for normal meshes or not.

@tdaven
Copy link
Contributor Author

tdaven commented Oct 3, 2024

There is still something wrong. The multi mesh seems to be getting dynamic shadows where the mesh instances are not. Will investigate further.

if (multi_mesh) {
// By default, get_meshes returns an Array with the transform first where the code
// below expects the mesh first. Request the order swapped so baking works.
bmeshes = multi_mesh->get_meshes(true);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of passing a parameter to get_meshes, I would just swap the order here. Since this function is the one with unique expectations, its better to keep the complexity here, instead of spreading it to MultiMeshInstance

@clayjohn
Copy link
Member

clayjohn commented Oct 3, 2024

There is still something wrong. The multi mesh seems to be getting dynamic shadows where the mesh instances are not. Will investigate further.

There is code to register the Lightmap with a MeshInstance when the Lightmap is loaded. That is probably missing MultiMeshInstances. I would start by looking here and making sure the lightmap is registered properly:

void LightmapGI::_assign_lightmaps() {

@tdaven
Copy link
Contributor Author

tdaven commented Oct 5, 2024

There is still something wrong. The multi mesh seems to be getting dynamic shadows where the mesh instances are not. Will investigate further.

There is code to register the Lightmap with a MeshInstance when the Lightmap is loaded. That is probably missing MultiMeshInstances. I would start by looking here and making sure the lightmap is registered properly:

void LightmapGI::_assign_lightmaps() {

Yeah, doing this has stopped the dynamic shadows but still not working entirely. I think is a problem where its not using the lightmap possibly because something is missing to tell it to use UV2. Still investigating what is going wrong. I tried looking at gridmap but it seems entirely broken for lightmap_gi as well for similar reasons.

@tdaven tdaven marked this pull request as draft October 23, 2024 05:20
@tdaven
Copy link
Contributor Author

tdaven commented Oct 23, 2024

This message was pasted to the rocket chat thread. Pasting here in-case its helpful for others.

I did a bit of investigation with gridmaps and I don't think this can be implemented like gridmaps. Gridmaps do have some uses of multimesh in the code, but it hardly used at all, especially for lightmaps. Everytime i've followed how grid map does it, you just get duplicate geometry. This is because gridmap spawns meshs when you bake the meshes.

I have a branch which does not use multimesh at all here (gridmap-no-multimesh). This branch won't display anything when editted, until you bake lightmaps. When it makes the baked meshes, it spawns new meshes. You can see the place this mostly happens here(gridmap mesh instancing). It also does some of this elsewhere to update. Its unclear to me where the multimesh stops drawing but it does. This was further confirmed by instrumenting the code and using the debugger. In the state mentioned above where the grid doesn't have baked meshes(and it doesn't show things properly), the multimesh is used and you can see it being drawed(assuming you're not on my no multimesh branch), but as soon as you bake meshes, it stops and instead you are only drawing mesh(s) made during baking linked above.

Here is an image from my no multi-mesh branch showing that it still renders and with working lightmaps:
image

@tdaven
Copy link
Contributor Author

tdaven commented Oct 23, 2024

This my latest push I have basic support implemented for lightmapgi and multimesh. Its not complete though and there are number of things I plan to revisit. But it does work across all three renderers and produces identical results.

Small test scene. One side is made with individual mesh instances, the other is multi-mesh. They look identical as desired. Multimesh is on the left in case you are wondering.
image

@tdaven tdaven force-pushed the fix-56027 branch 7 times, most recently from aff7cb4 to 7d6a677 Compare November 12, 2024 04:31
@tdaven tdaven marked this pull request as ready for review November 12, 2024 05:29
@tdaven tdaven requested review from a team as code owners November 12, 2024 05:29
@tdaven
Copy link
Contributor Author

tdaven commented Nov 23, 2024

@Calinou, I think I have fixed the issue you hit plus a few others as well (motion vectors where not being handle properly after I had cleaned up code).

image

I had thought those dark cubes were a problem but I believe they just have basically a negative scale so they are inside out. Non-lightmap lighting is also dark on them.

You'll want to re-import the the MRP project though. The main defect was a buffer copy which still had an offset (from one of the way I tried implement this) which resulted in no the entire buffer being copied which may have resulted in the project not saving the transforms correctly.

I also added menu items to view uv1/uv2 plus unwrap for lightmap since it was frustrating not having them. The code is mostly copy pasted from mesh_instance_3d. Not idea but it definitely improves the work flow. This bit could always be strip out to its own PR or maybe some common class both could use.

@tdaven tdaven force-pushed the fix-56027 branch 2 times, most recently from 9252a8d to ab6eda9 Compare November 25, 2024 02:34
@tdaven tdaven requested a review from a team as a code owner November 25, 2024 02:34
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 as expected in all rendering methods. Code looks good to me at a glance (other than the style changes I requested above).

@tdaven tdaven force-pushed the fix-56027 branch 3 times, most recently from 44dcdea to ffa8161 Compare December 13, 2024 02:38
@akien-mga akien-mga requested a review from clayjohn December 14, 2024 17:15
@tdaven tdaven force-pushed the fix-56027 branch 3 times, most recently from 69bb5de to a45a539 Compare December 19, 2024 01:31
@michaelharmonart
Copy link

super excited for this! awesome work

@clayjohn clayjohn modified the milestones: 4.4, 4.x Jan 2, 2025
@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 19, 2025
@tdaven tdaven force-pushed the fix-56027 branch 2 times, most recently from 3daf730 to 8428a2c Compare March 25, 2025 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:rendering topic:3d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vulkan: LightmapGI does not take MultiMeshes into account for baking
6 participants