-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
[3.x] Discrete Level of Detail #85437
Conversation
9ee3a49
to
d7ecf63
Compare
83f98d5
to
4f17acf
Compare
4f17acf
to
52639d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to see the viability of this for 3.x. There's no direct equivalent in 4.0, so it's difficult to judge.
(I wholeheartedly appreciate removing the confusing properties that never had any effect, myself)
fba7a66
to
4f9b903
Compare
Even though they don't do anything I'd say removing these properties is breaking compatibility, they still set values so nominally someone could be using them for their own implementation, right? |
To my dismay, yes. And they're not duds, either. They do set something in the VisualServer (even if it doesn't actually do anything). May be worth seeing some way to keep them out of view while still being functional |
See #57196 . We planned to remove these properties, but it was only delayed due to #53778 , which is superseded by this PR. |
It's been so long that the likelihood of these properties being used by someone out there is... still low, but not impossible. The documentation was never even changed to state that they may be removed in the future. The PR is still compatibility-breaking, but... pretty small all things considered, and more welcoming than not I would say. |
It is possible it was used by e.g. an addon, but it's fairly simple to update an addon, and we shouldn't let it hold up core development imo. Banditing existing non-working properties for an addon must be considered "subject to change" and caveat emptor. Leaving these non-working properties / functions in place just in case is likely to create confusion if there is now working LOD functionality, which doesn't use them. |
Just because it breaks compatibility doesn't mean this shouldn't be merged in my opinion, at least. |
c344cc2
to
b8e8bb2
Compare
Small update:Looking at @Calinou 's addon I realised he correctly points out that there is a problem caused by running the LOD system in the editor - the changes to visibility as you move the camera can cause unwanted diffs in the scene files. The addon simply turns off LOD in the editor to avoid this problem (and that is an option too), but it would be nice to allow it, as it previews quite nicely what will happen in game, and allows interactive adjustment of ranges. Instead here I've added a simple system in the editor saving to notify the (If preferred, I am equally happy to simply turn off LOD in the editor for now and leave this modification to a later PR where it can be more easily bikeshedded.) More
|
b8e8bb2
to
cfcf71d
Compare
cfcf71d
to
2bb2392
Compare
There was a problem hiding this 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 both GLES3 and GLES2. Other than the UI nitpicks below, it seems to work pretty well.
I'll look into designing an icon for the LOD node.
Testing project: test_lod.zip
- When setting up a new LOD system, the visibility for LOD's child nodes in the scene tree dock is only updated when you save the scene. Before I saved the scene, all nodes were visible even though their distance was already configured:
- Child nodes' visibility changes while in the editor and when saving, which can be distracting if your node is expanded:
simplescreenrecorder-2024-02-09_22.35.31.mp4
Maybe we should hide the visibility icon for children of a LOD node that inherit from Spatial, as there's no point in manually toggling visibility for those in the editor (changes will instantly be reverted).
- LOD update speed seems a bit slow when you have lots of instances. This might be an issue if your camera moves fast, as low-detail LODs will remain visible for a while after the camera gets close to objects. Is there a way to speed up the update rate (at the cost of increased CPU utilization)?
That would be fantastic. 👍
I'll have a look at this, it's not stop ship, but could be a quality of life improvement.
I'll have a look whether this is possible, no idea where the visibility icon is handled. Can also see pros and cons to this. An alternative (which I could easily put in) is a setting in the editor to turn on and off LOD in the editor, to prevent it becoming distracting.
Yes, this is exactly what the I was worried it would be overkill for users, but it does allow you to set exactly this update rate (by choice of 5 priority queues), and if you have noticed this so quickly, then users will no doubt find it useful. Maybe on reflection in the user interface it could be called BTW, what are the thoughts on whether this should be called "LOD" or "LevelOfDetail"? I'm 50/50 on this. We do use some acronyms like HTTP and IK, but I don't mind either and it's real easy to change now (but much harder once merged!). UPDATE: |
0309c9f
to
18942c0
Compare
Ok I've fixed it up so now in the editor, if This is good in some ways, but it's rather frustrating when you do try and change the visibility of the children and nothing happens, so I'm not entirely sure about it. Perhaps we can improve the user experience here somehow. Maybe an error warning in the output.. 🤷♂️ (EDIT : Have added warning message) It would be nice to ideally grey out the visibility icons for the user experience, however, this may end up being rather complex and bug prone so might be better for a followup PR (plus it would be nice if we can get this complete this weekend as hp I believe is intending to make the next 3.6 beta next week, so leaving some finer details is fine imo as long as we are happy with the general approach). I've also added code to completely remove |
0b07076
to
26384c7
Compare
Here's an attempt at an icon (it aims to represent a low-detail object, i.e. a circle turned into some kind of hexagon): icon_l_o_d.zip I've made the sides thicker so it looks more different from the Spatial icon. |
26384c7
to
2a4e845
Compare
Fantastic! Thanks. I'll add it to the PR. 👍 I also managed to get the visibility icons to grey out in the scene tree dock. It actually looks pretty good now and is far simpler than I thought it would be, just a few lines of code. This works much better than the warning message, because it is obvious to the user that clicking on the greyed out visibility will not alter it. |
2a4e845
to
47954be
Compare
47954be
to
2078c6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested the latest revision of this PR and it works as expected.
There are still a few areas of improvement, but these can be left for future PRs:
- LOD nodes with no valid children or only 1 valid child could emit a node configuration warning, so you know that at least 2 valid child nodes are needed for LOD to have an effect. (A valid child node is one that extends from Spatial.)
- When initially loading a scene, LOD is progressively applied with the selected priority, which can make it obvious in certain cases:
simplescreenrecorder-2024-02-10_19.21.43.mp4
Watch the video at 0.5× speed by right-clicking it to make it more noticeable.
Perhaps LOD priority should be forced to its maximum value for a few frames when the engine initially loads. That said, polished games generally don't immediately boot in a playable scene (they generally hide things with loading screens), so it may be a non-issue.
// Allow a generic mechanism for the engine to make changes prior, and after saving. | ||
if (editor_data.get_edited_scene_root()->get_tree() && editor_data.get_edited_scene_root()->get_tree()->get_root()) { | ||
edited_world = editor_data.get_edited_scene_root()->get_tree()->get_root()->get_world(); | ||
if (edited_world.is_valid()) { | ||
edited_world->notify_saving(true); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hook could be really useful to have in editor plugins and @tool
scripts, also in 4.x 🙂
There are multiple instances where I've wanted to make editor-visible changes that aren't serialized to scene files outside of LOD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I was surprised there wasn't something like this already. Although it may have to be done carefully in some cases to avoid race conditions.
Add scene side discrete level of detail. New node `LOD` for UI, and `LODManager` within `World` for automatically updating child visibilities based on distance from cameras.
2078c6b
to
1b5fa74
Compare
Yes, as you say, easy to do in follow up. 👍 |
Thanks! |
Woot woot, issue closed after 4 years! |
Add scene side discrete level of detail.
New
LOD
node for UI.LODManager
added withinWorld
for automatically updating child visibilities based on distance from cameras.Supersedes #53778
Fixes #40784
Demo Projects
Examine in the editor while moving viewer. Run, and use cursors to move camera.
testlod2.zip
Run, and use cursors to move. Contains multiple LOD nodes.
testlod3.zip
2024-02-10.08-39-55.mp4
Explanation
After exploring some fixes to #53778 I decided to try a scene side LOD system (like @Calinou 's addon).
LOD Node showing 3 example
MeshInstances
to be used as LODsLOD Node properties
Spatial properties
The old unused LOD properties are removed from
GeometryInstance
. Thelod_range
determines how much of the distance range is used by this LOD. So first child with 2 range will cover distance 0-2, second child with 5 range will cover distance 2-7, etc.Children of these
Spatials
will also automatically be shown / hidden as a consequence of the scene tree, as the LODs are automatically selected.Advantages & Disadvantages
There are advantages and disadvantages to using either scene side or server side approach:
Server side
Advantages
Disadvantages
visibility_parent
system inVisualServer
to work around inability to use the hierarchy.Scene side
Advantages
Disadvantages
Notes
VisualServer
andGeometryInstance
as these make no sense with this PR.NOTIFICATION_MOVED_IN_PARENT
#82248 which addschildren_changed
signal (so the calculation can be pushed on demand instead of pulled every time).affects_lod
property. Notably this is used for editor preview windows to prevent hidden windows affecting LOD.lod_enabled
property inGeometryInstance
as I realised it wasn't necessary. If users want to have nodes unaffected by LOD, they can either parent theLOD
node to aSpatial
and hang the nodes from this, or useSpatial
nodes as direct children ofLOD
(as these are notGeometryInstances
they and their children will be unaffected by theLOD
node).LOD
node properties to choose the update rate.view
menu in the 3d view.Discussion
This only took me today to do as an exploratory look. Discrete LOD can be done using an addon, but there seems some interest in a core solution so I put this up as an option.
Note that this also won't deal sensibly with
MultiMeshes
(as these are dealt with as a block rather than performing LOD on each instance within theMultiMesh
). If there is demand we could maybe add functionality to manage multimeshes, perhaps for vegetation, where otherwise drawcalls would be a bottleneck. These would probably have to be dealt with quite differently internally, although the user interface could probably be similar / shared.