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 MapSOCorrectWrapping removing stock MoHole #120

Closed
wants to merge 3 commits into from
Closed

Fix MapSOCorrectWrapping removing stock MoHole #120

wants to merge 3 commits into from

Conversation

R-T-B
Copy link

@R-T-B R-T-B commented Feb 5, 2023

Just blacklists the fix from a body named Moho. Thought it might be handy.

Just blacklists the fix from a body named Moho.  Thought it might be handy.
Fix biome and heightmap texture wrapping from pole to pole, even on Moho, This may wipe out the Mohole which is based on said bug, so it's an option.
Implement support for prior option.
@gotmachine
Copy link
Contributor

Mmmh... This is quite fragile...

First, this shouldn't be checking the config on every call. ConstructBilinearCoords() is a performance critical method, this add a significant overhead, easily avoidable by putting the parsed state in a static field.

But main issue is that there is no guarantee that in the call stack leading to a MapSO.ConstructBilinearCoords() call, the MapSO instance is matching FlightGlobals.currentMainBody. There are multiple stock API methods where this can happen (and I'm pretty sure they are used in such a way by some mods), and if remember right even PQS generation isn't always happening on the current main body either.

MapSO instances should be kinda static once PSystemSetup is done (even with Kopernicus I think ?), so it should be possible to get static references to the ones matching stock Moho and do direct reference comparisons. MapSO has a _name field and derive from ScriptableObject, which also has a name property, both are populated (at least for stock bodies), but this don't feel like a 100% reliable check either. A safer way to ensure we're targeting the right MapSO instances would be to compute a strong hash from the byte[] _data field, and store the hashes for the stock Moho MapSO instances as constants to compare what we get at runtime.

I will investigate how to do this :)

@R-T-B
Copy link
Author

R-T-B commented Feb 5, 2023

Interesting. I had no idea it fired that often. Anyhow, I leave this idea in your evidently capable hands.

One thing to keep in mind about data hashes vs simple name comparisons though: we have no idea if that will work with say, a rescaled moho, do we?

@gotmachine
Copy link
Contributor

gotmachine commented Feb 5, 2023

Interesting. I had no idea it fired that often

It's basically firing for every PQ mesh vertex. So millions of times. Just the fact that we are patching it already has a significant impact on performance.

One thing to keep in mind about data hashes vs simple name comparisons though: we have no idea if that will work with say, a rescaled moho, do we?

MapSo is essentially the raw texture data. I don't know how rescaling bodies work but I doubt it's by rescaling the texture itself.
Anyway, I won't do a real hash check, that would add too much overhead to loading, I will just check the name and a few bytes in the array.

And on a side note, I don't really like that we are doing this patch at all. Like some other cases we discussed, this is a Kopernicus specific issue, and it should be handled by Kopernicus, not by KSPCF. I just did some limited testing, and this patch in fact significantly alter the poles terrain on all stock bodies, no only Moho. Even with the fix I'm making, it's difficult to actually fix everything reliably because Kopernicus itself is calling those methods during its PSystemSetup override. This for example result in the Mohole anomaly object being slightly misplaced when Kopernicus is installed.

I don't argue that the stock code has a bug (which Squad decided to turn into a feature), but these kind of issues are exactly why I started KSPCF with the mission statement that its changes should be as seamless as possible in stock, and to let modded KSP issues be handled by mods, especially when there is already a well established mod handling something.

@gotmachine
Copy link
Contributor

Closing this, see #121

@gotmachine gotmachine closed this Feb 5, 2023
@R-T-B
Copy link
Author

R-T-B commented Feb 5, 2023

Closing as you did your own fix. EDIT: or you can lol.

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.

2 participants