Page MenuHomeWildfire Games

Discrete LOD
Needs ReviewPublic

Authored by Stan on Nov 24 2019, 8:14 PM.

Details

Reviewers
Angen
Summary

Implementing Discrete Level of detail for models based on the distance from camera.

Before submitting model to the renderer, it calculates distance from camera. Based on that computed distance, is switched to another actor of the model.

We avoid switching too much by comparing the lods and only reloading the actor when necessary.

Current distance to switch models is hardcoded and kind of small for showcase (maybe it is enough, dont know actually).

ToDo:

  • Decide whether the option is for v1 or v2 (Art work will not be done in this patch)
  • Find a way to get the configuration settings
  • Maybe using a logarithm for the distance algorithm

Another usage: Hyrule Conquest would like to have that.

Test Plan
  • Run the following steps.
    • Open Atlas
    • Place the themistocles actor
    • Zoom in and out and notice the actor change
  • Compare performance.
  • Agree with the naming convention

Event Timeline

Angen created this revision.Nov 24 2019, 8:14 PM
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Nov 24 2019, 8:14 PM
Stan awarded a token.Nov 24 2019, 8:16 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/639/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1155/display/redirect

elexis added a comment.EditedNov 24 2019, 8:27 PM

The intervals should probably not be buried in the code but specified somewhere, probably ideally in some model related art file or template (and ideally support more than two levels).
Perhaps std::pair<CStr, CStr>("lod", "low")) / selections can be constructed only once per program launch instead of once per call.
{ on a separate line.
Hiding projectiles after a set distance means it will fail for some projectiles of a given size, so it should be moved to data files as well.
It would be a good feature, although I guess in the average case only high level of detail will be shown, since the camera is top down on average?

Angen planned changes to this revision.EditedNov 24 2019, 8:49 PM

move interval to each actor
check distance only if actor defines that it has lod versions
disable/enable lod from graphic options
remove projectile manager changes as not exactly related

Angen added a comment.Nov 24 2019, 8:50 PM

It would be a good feature, although I guess in the average case only high level of detail will be shown, since the camera is top down on average?

With reasonable distance one can have high level models with close camera and low level in normal

vladislavbelov added inline comments.Nov 24 2019, 9:01 PM
binaries/data/mods/public/art/actors/structures/athenians/civil_centre.xml
28 ↗(On Diff #10410)

I think it'd be better to use numbers here, because it's easier to pick (no need to store strings in C++) and sometimes we need more levels of LODs and then we'd have a problem with new naming.

source/simulation2/components/CCmpProjectileManager.cpp
378 ↗(On Diff #10410)

It's not needed to calculate inverse of a camera matrix for each model. For many models it might be noticeable for performance (see implementation of GetInverse).

source/simulation2/components/CCmpUnitRenderer.cpp
74

What does the LOD name mean? It's not obvious from the name.

nani awarded a token.Nov 24 2019, 9:28 PM
Stan added a comment.Nov 25 2019, 2:27 PM

Lods should be in the template visual actor. Else actors will become a great mess. Also there will be warnings everywhere or missing props.

Good feature, indeed the implementation is relatively straightforward.

You probably want to make hardcoded numbers into class constants or configurable constants (or indeed, an option).

source/simulation2/components/CCmpProjectileManager.cpp
382 ↗(On Diff #10410)

This definitely needs to be a different check. Ideally, you would take the AABB, calculate how many pixels across it'd be, and possibly skip if the # of pixel is significantly less than 1.

Very good idea to work on that :) .
This needs some work before it can be committed, but nothing too huge I think.


On the LOD side, you have some code repetition and Vladislav said, you should not recalculate the camera matrix every time. Maybe add it to "Frustrum" (dunno if that makes sense), or pass it as an argument.

This is skippable, but given that you're doing the same LOD logic (to an extent) in different components, it sounds like a good idea for a header-only helper (basically just a function that takes all proper arguments and outputs the correct LOD). Since this is strictly graphics, it should go in graphics.

I think you should implement a "too far" culling. Ideally, take the bounding box into account and compute a sort of "size on screen", and discard if too low.
This would be particularly useful for props and actors (such as grass). It might thus make sense to implement your LOD directly in SubmitRecursive as well, for props.


With the above said, my main concern is that you're going to be running "ReloadActor" basically every frame. This is wasteful and kind of negates the performance gain of LOD. Further, we're manipulating strings and std::map, and that's slow.
Using variants is clever, but it won't work well for groups that already have named variants. I admit most mesh-only groups don't, but it's a problem.
I don't really have a good way around this... The simplest solution I can think of is to hardcode different actors in the VisualActor based on LOD levels, and then switch at runtime. It's kinda meh to do it in templates though, I think, and it forces creating entirely new actors (that might be OK though, because as elexis said in the general case our cameras don't really experience that much perspective).
The alternative would be creating a cleverer system to create objects, but that sounds entirely non-trivial :P

I'm not sure you actually should implement "low-high" switch for now. If you can already implement a working, efficient LOD that hides units and props when they're too far to see, we'll have something useful.

source/simulation2/components/CCmpUnitRenderer.cpp
485

Don't implement "behind the camera" culling.

Since you don't actually account for the entity BB, you'll fail occasionally and units will "pop-in". Further, culling is already handled, so this is just double-work.

Stan added a comment.May 23 2020, 6:18 PM

Very good idea to work on that :) .
...
It's kinda meh to do it in templates though, I think, and it forces creating entirely new actors (that might be OK though, because as elexis said in the general case our cameras don't really experience that much perspective).

I think it's better to do it in templates for the current actor system is messy enough, and it doesn't give much flexibility if you do it in actors.
Different Lods might have different animations, props and variants (eg less particles, lower poly sword, no fallback animation, simpler rigs) For instance, helmets wil never fit on low poly meshes.

Defining it in templates allows to separate concerns, and reduce the complexity of such files. It does require to introduce new templates though.
One could define a {lod} prefix by default none, with verylow, low, no prefix, high, veryhigh, ultra actors being picked depending on what's available.

<actor>{lod}{civ}_infantry_spearman</actor>

The only downsides is that it doesn't allow for a separation of concerns between the renderer and the simulation, but we don't currently have the resources to generate automatic lods

I made some profiling on the new map oceanside using athenians archers. I noticed realy big performance improvements when zooming out.
Test conditions: oceanside map, athenians archer, no moving of the units, saved at 4:30min, completely zoomed out, no moving of the camera from it's start position (only zooming out)


profiler2 frame: red old, green patched.


profiler2 old


profiler2 patched

Are there any plans to finish and commit this patch?

Stan added a comment.Nov 11 2020, 12:07 AM

@OptimusShepard is it the same if you disable the props?

I suspect <prop actor="props/units/quiver_greek_back.xml" attachpoint="back"/> to be particularly slow.

In D2440#135335, @Stan wrote:

@OptimusShepard is it the same if you disable the props?

I made a new profiling with deactivating instead of the patch, absolutely the same.

I suspect <prop actor="props/units/quiver_greek_back.xml" attachpoint="back"/> to be particularly slow.

You're right, this is the main point.


profiler2 frame: red patched, green only quiver greek back deactivated, blue old
I guess, trees could be interesting too.

Stan added a comment.Nov 11 2020, 12:59 AM

I'm gonna push some change, can you try again afterwards ?

In D2440#135338, @Stan wrote:

I'm gonna push some change, can you try again afterwards ?

Sure.

Stan added a comment.Thu, Jan 7, 2:21 PM

How hard would it be to switch in the template instead? Applying a visual actor change?

The problem with doing it with the actor is while it works for static meshes, it doesn't work so well for animated ones, because ihnerited variants for animations (which you might want to replace with simpler, or no animation at all.

Same for switching props, which usually are in variants.

Stan commandeered this revision.Sat, Jan 9, 1:55 PM
Stan added a reviewer: Angen.

Commandeering with @Angen's permission.

Stan updated this revision to Diff 15073.Sat, Jan 9, 2:10 PM

New version, with dynamic lod number, which allows for completely independant actors

Angen added a comment.Sat, Jan 9, 2:18 PM

would this work ?

source/simulation2/components/CCmpVisualActor.cpp
591

m_BaseActorName.concat("_" + lod_number) ?

Angen added a comment.Sat, Jan 9, 2:22 PM

ok, frogot .xml is part of template

Vulcan added a comment.Sat, Jan 9, 2:25 PM

Build has FAILED

builderr-debug-gcc7.txt
In file included from ../../../source/lib/precompiled.h:74,
                 from ../../../source/pch/simulation2/precompiled.h:19:
../../../source/simulation2/components/CCmpVisualActor.cpp: In member function 'virtual void CCmpVisualActor::SetLevelOfDetail(u8)':
../../../source/simulation2/components/CCmpVisualActor.cpp:267:24: warning: comparison is always true due to limited range of data type [-Wtype-limits]
   ENSURE(levelOfDetail >= 0 && levelOfDetail <= m_MaxLevelOfDetail);
          ~~~~~~~~~~~~~~^~~~
../../../source/lib/debug.h:295:8: note: in definition of macro 'ENSURE'
   if(!(expr))\
        ^~~~
builderr-release-gcc7.txt
��
_: internal compiler error: Segmentation fault
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-8/README.Bugs> for instructions.
make[1]: *** [engine.make:477: obj/engine_Release/SoundGroup.o] Error 1
make: *** [Makefile:119: engine] Error 2

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/4394/display/redirect
See console output for more information: https://jenkins.wildfiregames.com/job/docker-differential/4394/display/redirectconsole

Vulcan added a comment.Sat, Jan 9, 2:34 PM

Build is green

builderr-debug-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgraphics_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas_dbg.a(precompiled.o) has no symbols
ld: warning: text-based stub file /System/Library/Frameworks//CoreAudio.framework/CoreAudio.tbd and library file /System/Library/Frameworks//CoreAudio.framework/CoreAudio are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/F

See https://jenkins.wildfiregames.com/job/macos-differential/2731/display/redirect for more details.

Stan updated this revision to Diff 15075.Sat, Jan 9, 3:11 PM
Stan marked 2 inline comments as done.

Small changes fix naming. Maybe we should use VFSPath everywhere as it's actually the correct type

Vulcan added a comment.Sat, Jan 9, 3:20 PM

Build has FAILED

builderr-release-gcc7.txt
��
_: internal compiler error: Segmentation fault
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-8/README.Bugs> for instructions.
make[1]: *** [engine.make:477: obj/engine_Release/SoundGroup.o] Error 1
make: *** [Makefile:119: engine] Error 2

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/4396/display/redirect
See console output for more information: https://jenkins.wildfiregames.com/job/docker-differential/4396/display/redirectconsole

Vulcan added a comment.Sat, Jan 9, 4:35 PM

Build is green

builderr-debug-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgraphics_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas_dbg.a(precompiled.o) has no symbols
ld: warning: text-based stub file /System/Library/Frameworks//CoreAudio.framework/CoreAudio.tbd and library file /System/Library/Frameworks//CoreAudio.framework/CoreAudio are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/F

See https://jenkins.wildfiregames.com/job/macos-differential/2733/display/redirect for more details.

Stan edited the summary of this revision. (Show Details)Sat, Jan 9, 4:47 PM
Stan edited the test plan for this revision. (Show Details)
Stan edited the summary of this revision. (Show Details)