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

PQS.sphere.sx structure is non-linear #41

Closed
R-T-B opened this issue Jun 1, 2022 · 3 comments
Closed

PQS.sphere.sx structure is non-linear #41

R-T-B opened this issue Jun 1, 2022 · 3 comments
Labels
kspBug Identified KSP issue

Comments

@R-T-B
Copy link

R-T-B commented Jun 1, 2022

The bug can be summarized as follows:

PQS.sphere.sx is supposed to be a variable of linear structure spanning from 0 to 1 indicating a longitude point (similar to how PQS.sphere.sy functions properly for latitude). It does not. Range 0-0.75 behaves normally, but the final quadrant of longitude is in fact migrated nonlinearly into the -0.25 to 0 part of the value. This causes obvious issues, and the root cause is not only diffilcult to diagnose, but if truly corrected at the source probably would break other game facilities that have long worked around this bug.

The only known bug from this is that Kopernicus LongitudeRange values ignore the final quadrant, as a direct result of this nonlinear value representation. A patch can be made to fix this in PQSLandControl. In that class, there is this line:

this.vLon = this.sphere.sx + (double)this.longitudeBlend * this.longitudeSimplex.noise(data.directionFromCenter);

I propose that line be replaced with:

double correctedSphereSx; if (this.sphere.sx > 0.0) { correctedSphereSx = this.sphere.sx; } else { correctedSphereSx = this.sphere.sx + 1.0; } this.vLon = correctedSphereSx + (double)this.longitudeBlend * this.longitudeSimplex.noise(data.directionFromCenter);

That should work around the nonlinearity in the one known symptomatic instance, while not mucking about with any stock systems that may already work around the core bug. Feel free to provide your thoughts.

@gotmachine gotmachine added the kspBug Identified KSP issue label Jun 1, 2022
@gotmachine
Copy link
Contributor

gotmachine commented Jun 1, 2022

So, to be clear, we are talking about PQSLandControl.OnVertexBuildHeight() ?

This piece of code is indeed quite hard to follow. This pattern of having instance fields being used as local variables shared by multiple methods all over the place is really awful.

Despite this being quite weird, I think sx is designed to be in the [-0.25, 0.75] range. There are way too many places where this is used for this to be a mistake. Since this is mapping to texture UV coordinates, my interpretation is that there is a deliberate intent to have a 90° offset between the texture origin and the 0° longitude.

And in fact, when using those values as UV coordinates, the stock code always does val = Math.Abs(val - Math.Floor(val)) which is equivalent as your modified code to transform back the value to a valid [0, 1] UV range.

I think the actual mistake is indeed PQSLandControl.OnVertexBuildHeight() trying to get back a longitude value from the sx coordinates without accounting for that offset, so your fix makes sense from that PoV.

What I'm unsure of is how this will interact with the "intended" scatter placement. Fixing this like that mean whatever stuff is used to configure the distribution is working with a "the origin is at 90° longitude" assumption, but I guess that's already accounted for by people using this. Still, a confirmation that scatters are actually where they are supposed to be would be nice.

I will implement that fix and try to do a release ASAP.

On what this fix should be described, what is PQSLandControl used for ? Does it only affect scatters distribution or is the class used for other purposes ?

gotmachine added a commit that referenced this issue Jun 1, 2022
… a partial longitude range is defined in the PQSLandControl definition, see issue #41
@R-T-B
Copy link
Author

R-T-B commented Jun 1, 2022

Yes you got the right function. And looking at it I agree with everything you say... just seems very odd a choice of coordinate system.

Will test soon.

@R-T-B
Copy link
Author

R-T-B commented Jun 1, 2022

Forgot to answer one point: LandControl can do a bit more than scatters. It can do some terrain coloring effects amongst other things. Mainly scatters though.

gotmachine added a commit that referenced this issue Jan 30, 2023
… a partial longitude range is defined in the PQSLandControl definition, see issue #41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kspBug Identified KSP issue
Development

No branches or pull requests

2 participants