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 #59: Disable OrbitRenderer and MapObjects when not in map mode/tracking station #60

Closed
wants to merge 3 commits into from
Closed

Fix #59: Disable OrbitRenderer and MapObjects when not in map mode/tracking station #60

wants to merge 3 commits into from

Conversation

JonnyOThan
Copy link
Contributor

No description provided.

@gotmachine
Copy link
Contributor

I don't like how this is implemented. You never know what random plugin X is doing, so I'm always trying to minimize any behavior difference versus stock.

Calling LateUpdate from GameEvents callbacks will result in incorrect behavior. It probably doesn't cause obvious issues, but I prefer to avoid altering stock behavior when possible, especially when inducing subtle changes like that. Enabling/disabling components is also risky if anything else is relying on that.

I would suggest using a pooling approach with OrbitRendererBase.LateUpdate(), bailing out early if MapView.fetch.updateMap is false. This will be slightly less efficient, but much safer.

I'm a bit more inclined to allow disabling MapObjects, but I would suggest disabling the GameObject instead of the component.
And to do it using a MapObject.Start() postfix, then handling enabling/disabling all of them using the ScaledSpace.scaledSpaceObjects list, not using GameEvents but with a prefix on MapView.enterMapView() and a postfix on MapView.exitMapView().

As a general rule, when altering stock behavior, never rely on GameEvents. You have absolutely no control on the call order of GE delegates, and it is usually better to restore the expected stock state before the stock method firing the GE even starts.

Will poke at implementing all that, letting this open for now.

@gotmachine
Copy link
Contributor

Mmmh. Disabling MapObjects shoudl be done carefully.
ScaledMovement derives from MapObject and is responsible for positioning scaled space vessels/bodies, and that shouldn't be disabled.

@JonnyOThan
Copy link
Contributor Author

Yes, my first attempt deactivated the gameobjects but quickly (well not that quickly) realized that there were other components on those objects that need to run.

@gotmachine
Copy link
Contributor

What I mean is that OrbitRendererBase.objectMO can and will usually be a ScaledMovement instance. Those should not be disabled, as they are responsible for setting the position of scaled space bodies/vessels, and those are widely used for functional purposes (for example solar panel occlusion).

@gotmachine
Copy link
Contributor

gotmachine commented Jul 14, 2022

So, follow-up : I don't think it's even worth it to disable MapObject instances. The base class has actually no OnLateUpdate() implementation, only ScaledMovement is doing something, and we can't disable those.

Profiling with ~54 scaledspace objects (stock bodies + a bunch of vessels and asteroids) gives the following results :
image
This is 0.01 ms of gains for 54 objects, so extrapolating a bit, even for a game with 1000 scaledspace objects, that's less than 0.2 ms avoided (and probably much less in a non-debug game, remember that all managed runtime optimizations are disabled here).

The only significant gains are from bailing out early of OrbitRendererBase.LateUpdate() :
image

Given that disabling MapObject instances is inherently risky, I think I will only implement early bail-out in OrbitRendererBase.LateUpdate(), as this account for 99% of the perf gains and is quite safe to do.

@gotmachine gotmachine closed this Jul 14, 2022
@JonnyOThan
Copy link
Contributor Author

Er, this change doesn't disable all MapObjects - only the ones that represent the icons owned by OrbitRenderer. There are 5 of those per OrbitRenderer.

I'm seeing ~1.5ms in MapObject.LateUpdate in the profiler with about 1200 objects (bodies + vessels). The count per frame is about 6400.

@gotmachine
Copy link
Contributor

gotmachine commented Jul 14, 2022

this change doesn't disable all MapObjects - only the ones that represent the icons owned by OrbitRenderer. There are 5 of those per OrbitRenderer.

Again, one of those 5 MapObjects (objectMO) is actually a ScaledMovement instance, which shouldn't be disabled, and is also responsible for 95% of the overall MapObject.LateUpdate() load. The 4 others, which are base MapObject instances don't do anything (their OnLateUpdate() implementation is empty).

Said otherwise, you can't avoid the MapObject.LateUpdate() load unless you're willing to break stock functionality and likely a bunch of mods relying on scaledspace objects being where they should be. ScaledSpace representations of bodies and vessels are independent from the map view, and are used functionally for other things than the map view / planetarium.

And side note, the map objects aren't "owned" by OrbitRenderer, they are in fact are in completely different GameObject hierarchies and "managed" by different components depending on the current scene. It's quite messy.

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

Successfully merging this pull request may close these issues.

None yet

2 participants