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 LightmapProbe placement workflow #7938

Open
jcostello opened this issue Sep 30, 2023 · 30 comments
Open

Improve LightmapProbe placement workflow #7938

jcostello opened this issue Sep 30, 2023 · 30 comments

Comments

@jcostello
Copy link

Describe the project you are working on

3D Lighting

Describe the problem or limitation you are having in your project

Currently, working with Light Probes in large scene is tedious.

There is only who ways of working with Light Probes:

  1. Subdivision
  2. Place Manually

Subdivision in large scenes light the TPS demo does not do much. Even the 32 subdivision is not enough to cover the level since there is too much space between the probes and subdivision doesn't work by distance. Also, it places probes where the player will never go

Place Manually is really painful and not very accurate. I found that in order to have proper indirect lighting on dynamic object, it should be enough probes and they have to be close enough to guaranty a smooth transition and a good result.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Something like a node (LightProbesGroup or something like that) that consist in an area (like the ReflectionProbes) where Light Probes can be generated. With a parameter by distance so it could generate as much probes as the area can have by a separation of distance (probably distance in xz and y to generate more probes in in xz than in y)

This way, we can generate probes in desired areas with desired density of probes.

Also, probes of different areas should't be linked together unless areas overlap (To generate an accurate result and prevent dynamic object to the light from undesired probes)

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

N/A

If this enhancement will not be used often, can it be worked around with a few lines of script?

A script could place probes but workflow will still be bad.

Is there a reason why this should be core and not an add-on in the asset library?

N/A

@Calinou Calinou changed the title Improve Light Probes workflow Improve LightmapProbe placement workflow Oct 1, 2023
@Calinou
Copy link
Member

Calinou commented Oct 1, 2023

I'm not sure if we should go back to making people manually place their probes, given how time-consuming that process is. It should remain a last resort, not something you have to do because the automatic generation is ill-suited in most scenarios.

Instead, it should be possible to guide the automatic probe system to only generate probes within the playable area. Maybe a NavigationMesh can be used as a helper here?

@jcostello
Copy link
Author

Something like a node (LightProbesGroup or something like that) that consist in an area (like the ReflectionProbes) where Light Probes can be generated.

@Calinou Here I was referring to that. Having something like an area where the probes can be generated automatically. I thought about NavigationMesh but is not always used in every scenario and I don't want to diverge the use of it for Lightmap. NavigationMesh should be only for navigation

I created a simple script that does that as a proof of concept
image

@jcostello
Copy link
Author

I made a script that places probes in a Vector3 distance from each other inside of a BoxShape as a POC.

image

Is hard to see in the image (and also hard to work). The workflow is tedious and doesnt help much because the probes are placed in places that doesnt make much sense. Also that many probes in the scene stalls the Lightmap bake. I dont see any errors but it never finish this part

Screenshot from 2023-10-01 10-27-16

Maybe its a good idea to implemente something like this to have them automatically and some way to restrict the interest areas.

https://www.youtube.com/watch?v=n3ACAjlhgJQ
http://graphics.cs.aueb.gr/graphics/docs/papers/Illumination_driven_Light_Probe_Placement.pdf
https://github.com/cgaueb/light_probe_placement

@WickedInsignia
Copy link

Even just having individually-placed probes under a single multi-use node (like the LightProbes entity in Unity) that only reveals probe placements/allows editing upon selection would be a lot better usability-wise than what we have now. Otherwise the scene becomes an unusable mess of nodes.
An intelligent volume that takes meshes into account would be great though.

I'm not sure if we should go back to making people manually place their probes, given how time-consuming that process is.

Automatic generation is effective in theory but not in practicality. Unless you have an extremely smart and well-thought-out system there's too many individual problems that need to be fixed manually. Sometimes moving a probe 2-3 centimeters off the ground is enough to solve an issue, and there are some common hacks (such as placing a probe far above or below the level as a fallback for GI issues) that an automated system would never think of doing.
Also, probes should only generally be placed in frequency in areas where the light changes drastically. When you're using completely lightmapped lighting, you also want to place most of your nodes at the edges of shadows. Trying to preempt that would make for an extremely finicky system.
Automatic distribution with the option for manual fine-tuning of individual probes afterwards would be best. If the automatic generation is a success, there's no need for the user to fiddle with the probes and they're all nicely tucked away in a single node without cluttering the scene unless called upon.

@jcostello
Copy link
Author

Even just having individually-placed probes under a single multi-use node (like the LightProbes entity in Unity) that only reveals probe placements/allows editing upon selection would be a lot better usability-wise than what we have now. Otherwise the scene becomes an unusable mess of nodes.

Can you elaborate a bit your proposal?

@WickedInsignia
Copy link

Sure! It's a little hard to explain when someone hasn't used the lightprobe system in Unity, but essentially:
Light probes are part of a Light Probe Group entity (an entity in Unity is mostly equivalent to a node in Godot).
This means they're all neatly hidden away until the Light Probe Group is Selected. When not selected, the Light Probe Group is a simple icon. When it is selected, all the probes contained in that entity become visible like so:
class-LightProbeGroup-9

If you click the "edit probes" button in the inspector you can then edit the probes by duplicating and moving them. There's no need for an "add new probe" button since every Light Probe Group gives you 4 probes to start with, and you can simply duplicate more from there.
The "edit probes" button is shown below, it has 3 boxes with two lines connecting them:
class-LightProbeGroup-20183

You can find more specific info on how the system works HERE.

This system is a lot less cluttered than Godot's current implementation. It doesn't need to be replicated exactly, but taking inspiration from some elements (such as a dedicated Light Probe Group node or hiding all light probes unless their parent node is selected) could be helpful.
Maybe light probes only become visible when a probe is selected? All the probes inside a parent node containing many nodes could become visible if only one inside the group is selected. That way you could have multiple node groups in a single scene for organization's sake.
That, or a unique Light Probe Group node could be created with specific controls for editing light probes.

@jcostello
Copy link
Author

Yeah, thats something similar of what I have in mind. Also probes now are visible when you select the lightmap, if not they are hide. But I agree that showing them when they are selected its a good idea. In summary:

  1. Have a node LightProbesGroup node
  2. Lightprobes will no longer be a single node
  3. Lightprobes can be selected, placed, moved and deleted inside the LightProbesGroup
  4. Triangulation of the probes should be inside the group and in editor time.
  5. Have the ability of generate a grid fill by distance of the probes just to have something initially to work?

@WickedInsignia
Copy link

WickedInsignia commented Oct 3, 2023

Yeah that sounds pretty usable to me. The novelty of the system will be the ability to auto-generate and then edit individual probes afterwards, that's something you always required a plugin for in Unity. Even just a basic 3D grid is better than nothing, with the user making a few adjustments here and there to move probes out of geometry or into more optimal positions.

Some sort of cue to tell the user when a probe is inside geometry would be useful as well. One of the biggest problems with probes in Unity is that they draw over all geometry (definitely preferred to being obscured) but don't signify when they're inside a mesh.
Maybe when obscured they show as a wireframe ball, but when fully visible they're shown as a shaded ball? It would make identifying bad probe placement a bit easier, or when they're halfway intersecting with a mesh.

@jcostello
Copy link
Author

I was checking out the unity legacy probes system and the new adaptive probe system. The way the new adaptive way works is in a grid distributed way and check for invalid probes. Also probes light the meshes per pixel to give a better result. That could be nice to have as well. With this last thing probes position wont need to be that precise and a grid would work most of the cases.

Also its would be awesome to bake probes separately from the lightmap since after baking the light, probe position may be adjusted

@Calinou
Copy link
Member

Calinou commented Oct 6, 2023

On second thought, a node that adds in a cuboid area is perhaps not a bad idea. When you have a level like this, Godot will generate a lot of probes outside the playable area but overall probe density remains low:

image

Lots of probes are also generated high up in the sky, which is wasteful if your dynamic objects barely leave the ground.

Light probes are part of a Light Probe Group entity (an entity in Unity is mostly equivalent to a node in Godot).

If we add a node that spreads LightmapProbes throughout an area, you could add LightmapProbe nodes as children to it.

Regarding lightmap probe display in the editor, I noticed a few areas of improvement:

  • The material's cull mode is set to Disabled instead of Back, so you can't see anything if the camera is inside a probe preview.
  • They don't receive direct light, so they don't accurately reflect how a dynamic object will look like in practice.
  • Perhaps they could fade in/out according to distance (when the camera gets very close, or far away, or both).

Partial x-ray display should be implementable using a second material pass that is transparent and disables the depth test. This one may be a good candidate for fading in the distance as to not look annoying in large scenes.

@jcostello
Copy link
Author

jcostello commented Oct 6, 2023

If we add a node that spreads LightmapProbes throughout an area, you could add LightmapProbe nodes as children to it.

If probes will only live inside a LightProbesGroup (to name it something) doesn't make much sense to have a LightmapProbe. I will suggest to have probes like gizmos and not like nodes.

They don't receive direct light, so they don't accurately reflect how a dynamic object will look like in practice.

This would be a great improvement

Perhaps they could fade in/out according to distance (when the camera gets very close, or far away, or both).

Yes to this

@Stwend
Copy link

Stwend commented Oct 7, 2023

In my opinion, something like this should not be core. Placing light/reflection probes has so many different uses cases for different games, the placement system would inevitably get huge, and every user would only need a fraction of it in their individual game.

I will suggest to have probes like gizmos and not like nodes

Strongly against this. Having a node based probe enables users to write arbitrary placement editor scripts.

@WickedInsignia
Copy link

Before we start launching into highly creative ideas it might be best to tackle what isn't working with the system as it exists right now, and lay down solutions out so we aren't inventing anything that doesn't improve the current system directly.
I'd like to start by breaking down the issues individually:

  • Selecting probes can be difficult, since you may accidentally select part of the scenery in the process.
  • They are always visible, each with their own icon and wireframe. This poses a problem since probes are usually used at high numbers for proper illumination of dynamic objects.
  • It's never shown what probes are affecting a dynamic object in-editor. The only way to see the volume/how probes connect to each other is by selecting the LightmapGI node.
  • They crowd up the outliner unless held in a parent node. Even then, there's not much utility to keeping each probe as a separate node because they're usually used in such high numbers. It's difficult to know what LightmapProbe88, LightmapProbe89 etc. are and simply easier to select/manipulate them in the scene editor.

Unity solves all these issues with its implementation. I hate to compare to a major package, but that's the reality of it.
These issues may be resolvable by taking cues from implementations that solve the problems we have, rather than reinventing the wheel entirely:

  • Contain probes in a single dedicated LightmapProbe node, rather than each probe as an individual node. This is similar to Unity's LightProbeGroup entity.
  • Show how these probes are connected once baked, but only when that node is selected rather than LightmapGI.
  • Only allow probes to be selected/duplicated/moved when that node is selected, or when a dedicated probe selection mode is entered.
  • Show which probes are affecting a dynamic object (and ONLY those probes) when that object is selected/moved in-editor.
  • Allow for an optional distance fade, to keep probe-heavy scenes readable while editing.
  • Allow for multiples of these nodes to communicate with each other, so that more than one can be used in any one scene.

This strategy may give us room to expand with more features, such as:

  • LightmapProbe nodes that allow for automatic generation within a specified domain. If more manual handiwork is required it could be converted to a standard LightmapProbe node, or it could communicate with a standard LightmapProbe node if only additional probes are required.

I feel that this is mostly what we've landed on with current discussions.
Let me know if this is off the mark! Just trying to get it all down so everyone's on the same page.

@Stwend
Copy link

Stwend commented Oct 7, 2023

Contain probes in a single dedicated LightmapProbe node, rather than each probe as an individual node. This is similar to Unity's LightProbeGroup entity.

I have to repeat myself, this is a really, really bad idea. Do not do this. This is not one of Unity's good features. It will forever limit every single user to only being able to use your system.

The LightmapProbe Node was created to avoid this specific issue:

https://docs.godotengine.org/en/4.1/classes/class_lightmapprobe.html
"By creating LightmapProbe nodes before baking lightmaps, you can add more probes in specific areas for greater detail, or disable automatic generation and rely only on manually placed probes instead."

//edit: After re-reading your poposal, it seems that you're suggesting an optional "Probe Parent" node instead of replacing the currently existing probes with a single blackbox probe container node. Such an approach would be a nice addition.

@WickedInsignia
Copy link

There’s no benefit afforded by adding lightprobe nodes as single nodes, instead of a grouped probe node system. If you want a single node, add a LightmapProbeGroup node and only one probe in that node.

Quoting Godot’s recommended usecase isn’t an argument that it’s a bad idea. That’s just an explanation for why Godot is the way it is, not why the alternative is bad. You can work exactly the same with a grouped system: you can add detail where necessary (just duplicate more nodes) or opt to automate, combine automated domains with standard groups, or convert the automated domain to a standard group to manually edit it.
Godot’s system simply isn’t well-suited to extensive lightprobe work and poses all of the issues I’ve mentioned above. Unity’s LightProbeGroup system is considerably less unwieldy and Godot’s system doesn’t offer any tangible benefits in the way it does things differently. That’s why I proposed something similar.

I’m not sure what you mean by “it will forever limit every user into only being able to use your system.”
Right now Godot users are forced into using a system that’s visually disruptive and tedious.
I understand and welcome objections, but I need to hear a little more reasoning on this point to understand it I think.

As I mentioned earlier in the thread, being able to add a single probe as a node should remain a feature. I just don’t anticipate anyone preferring that over a grouped system, and I’ve personally never wished for Unity to offer a single probe over the probe group it offers.

@Stwend
Copy link

Stwend commented Oct 7, 2023

Ah, we're kind of on the same page then. Thanks for the explanation, much appreciated.

I’m not sure what you mean by “it will forever limit every user into only being able to use your system.”

I misunderstood your system to be a "probe container blackbox" with no way of handling single probes on their own or manually altering/extending probe placement, similar to how UE4 did it a few years back with their Indirect Lighting Cache (I'm not sure if that's still the case today) and I thoroughly hated working with that system.

I'm not entirely sure if I fully grasp the benefits and motivation behind the new system:
The aim is being able to define an area:

  • in which light probes are automatically being placed according to that area's settings,
  • which hides its child probes unless selected/applies a distance fade to avoid clutter,
  • that offers visual debugging functionality
  • and allows for manual probe placement/editing.

Is that correct?

@WickedInsignia
Copy link

WickedInsignia commented Oct 8, 2023

To break it down, my proposal (and the proposal of some others here, I assume) is to add a LightmapProbeGroup node. This would either replace the LightmapProbe node, or complement it.
The LightmapProbeGroup node can contain any number of probes. Each probe is individually editable, but only if the LightmapProbeGroup node is selected or if a special selection mode is entered (the latter would be optimal).
Probes within different LightmapProbeGroups will all communicate with each other.

This works almost identically to Unity's LightProbeGroup entity, but adapted for Godot. For example, here's how Unity's system works:

LightProbeGroup entity when selected:
Lightprobegroup

The same entity in edit mode, where nodes can be duplicated and moved without the risk of selecting anything else in the scene:
LightprobegroupEdit

The same entity when it is not selected. It turns into a simple icon and hides all probes:
LightprobegroupUnselected

What happens when you select and move a dynamic object. The probes that affect it are shown for debugging:
LightprobegroupDynamic

There may also be an automated LightmapProbeGroup node added.
This allows for automatic distribution of nodes within a domain (such as the domain used to define a VoxelGI volume). Most likely based on parameters defined by the user (how dense, level of awareness of manifold meshes, vertical distribution etc.)
This automated node can be converted to a standard LightmapProbeGroup node if manual editing is desired. This would most likely be a one-way conversion.

In that way, all four points you've listed are correct.
The benefits are resolving the clutter and clumsiness of the current Godot probe system.

@jcostello
Copy link
Author

jcostello commented Oct 8, 2023

I like the idea. Also having all inside a LightmapProbeGroup could help to create the BSP in editor time so it doesn't take baking time and also create a valid one.
LightmapProbeGroups's probes should be in the same BSP?

@WickedInsignia
Copy link

@jcostello I'm not sure what you mean by BPS/BSP?

@jcostello
Copy link
Author

For what I understand BSP is a way of subdividing space in which it generates a tree like data structures, AKA it links the probes in a tree structure. After baking you will see the probes linked by a line when selecting the GPULightmap.
The problem is that this process is created when the lightmap is baked and not when the probes are placed manually.
Ideally this should be done inside the LightmapProbeGroup so problems can be detected and it doesn't take baking time. I think Unity does this in editor time

@WickedInsignia
Copy link

Yeah I think that should be live and calculated while editing. Seeing how the probes interconnect is pretty essential info when placing them. I forgot Godot doesn’t show that in real time until you mentioned it!

@jcostello
Copy link
Author

@WickedInsignia if you like you can create a new proposal that supersede this one we all the details we came up to this point, including this point

@WickedInsignia
Copy link

If you're fine with that, sure! I'll need a minute to pull everything together but will have it up as soon as I can.

@Calinou
Copy link
Member

Calinou commented Oct 24, 2023

I have a working implementation of a LightmapProbe rework that lets it create multiple probes (without additional nodes): https://github.com/Calinou/godot/tree/lightmapprobe-add-probe-groups
This should be 100% backwards-compatible with existing projects.

Cell size (defined in meters on each axis) is automatically adjusted so that the size is fully covered with probes, without any gaps or irregular spacing:

probes_grid webp

If cell size is greater than the size on any of the dimensions, only one probe is generated on that dimension, but it will be centered:

probes_line webp

The default behavior is to use a size and cell size that generates a single probe in the middle, so that backwards compatibility is preserved:

probes_single webp

A manipulation gizmo and undo/redo are available, with the usual box helper found in 4.2 and later. I haven't copied over the subdivision drawing code from the VoxelGI gizmo yet.

@jcostello
Copy link
Author

Very niceeeeeeeeeeeee, i love where this is going. Let me ask you a few questions:

I haven't copied over the subdivision drawing code from the VoxelGI gizmo yet.

What do you mean by this?

Probes internally works like manually placed probes?

Is there a possibility that make probes link in editor time, to solve potential problems before the lightmap bakes?

@Calinou
Copy link
Member

Calinou commented Oct 24, 2023

I haven't copied over the subdivision drawing code from the VoxelGI gizmo yet.

The VoxelGI gizmo draws a preview of the subdivisions on the gizmo's faces. This could be added to LightmapProbe so you get an idea of how far away probes are spread before you bake lights, without looking too intrusive.

@jcostello
Copy link
Author

The VoxelGI gizmo draws a preview of the subdivisions on the gizmo's faces. This could be added to LightmapProbe so you get an idea of how far away probes are spread before you bake lights, without looking too intrusive.

could we get a preview using fake probes spheres semitransparent instead?

@SpockBauru
Copy link

Hello! I released a plugin that handles LightmapProbes, the idea is to make a grid of lightmap probes and cut unwanted ones. Here's a video (with cute sound :3): https://www.youtube.com/watch?v=IdpsFQpvp2I

Unfortunately, it relies on collision to cut the unwanted probes. Ideally it would test the meshes themselves, but I didn't find a way to do it with GDScript. I tried RenderingServer.instances_cull_ray but it has 2 problems:

  1. It does not test the meshes themselves but rather the bounding box around the object.
  2. It's broken: #39197

Is there any way to test for the meshes in the scene in GDScript? Maybe some shader trick?

Speaking on shader tricks, the natural evolution would be to cut based on light information, is there any way to get lighting information in GDScript? Something similar to Unity's SphericalHarmonicsL2.Evaluate where you get the light information from a given direction, or some shader code to get the light information back to CPU.

About the current proposal, would be really nice a way to know from which probes the object is getting the lighting, something like WickedInsignia proposed. I believe this would benefit on debugging errors (like on issue #82642):

ProbesPreview.mp4

@Calinou
Copy link
Member

Calinou commented Jan 4, 2024

Is there any way to test for the meshes in the scene in GDScript? Maybe some shader trick?

It's likely feasible to use SurfaceTool or similar, but it'll be slow in complex scenes. See godotengine/godot#83420.

@patwork
Copy link

patwork commented Mar 8, 2024

Hi, in my opinion the ability to add multiple separate lightprobe volumes to a scene and to be able to move individual ones around is very important for proper lighting of dynamic objects, especially in situations where the lighting in different parts of the scene varies a lot.

An example from Unity:

https://youtu.be/hMnetI4-dNY?t=2582&si=aHglM7SmkKDrxS30

Zrzut ekranu z 2024-03-08 11-03-21

Zrzut ekranu z 2024-03-08 11-04-18

Unity also has the ability to detect where automatically placed lightprobes are incorrect (for example inside geometry):

https://www.youtube.com/watch?v=DlxuvvYZO4Q&t=1037s

Zrzut ekranu z 2024-03-08 11-05-17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants