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

Workaround to fix wrong position of packed vessels #2818

Merged
merged 3 commits into from
Dec 2, 2020

Conversation

marianoapp
Copy link
Contributor

There's a bug in KSP where it returns the wrong position for packed vessels. Instead of returning the current position is returning the position where it will be in the next simulation frame.

Ideally we shouldn't be working around KSP bugs but this one is severe enough that I think merits the effort, and if KSP ever fixes the underlying issue it's easy to rollback.

poserror

In the image there are two arrows, the red one points to the position reported by KSP and the green one to the position using the workaround. Getting closer to the target vessel caused it to unpack and then the red arrow started pointing to the correct position.

In the example above the workaround was done in a kOS script BEFORE applying this patch and that's why I had access to the "wrong" position reported by KSP.

@nuggreat
Copy link

So I have some questions about this.

Does the issue still exist when on rails if so then your fix would cause the same problem just in a different circumstance?

It also looks like you are hard coding 0.02 as the time step what about differing time steps that occur at different levels of physics warp wouldn't that simply cause the same issue as you are only correcting for the 1x time step?

@marianoapp
Copy link
Contributor Author

I would need to test how it behaves when the target is on rails, but you're right about the time step, that was an oversight.

@mgalyean
Copy link

Given that the patch is generating the correct location, and KSP is going to generate the incorrect one for now either way, it shouldn't be hard for the code to recognize if/when KSP fixes the underlying problem by comparing the fixed location with the KSP reported location. In which case it could automatically bypass the fix code until the code could be properly revisited and re-optimized. This would keep it from breaking noticeably and turn it into a minor maintenance task done at leisure rather than a fire to be stomped out. Just a thought

@nuggreat
Copy link

Another question does this only affect SHIP:POSITION or does it also affect the position of parts?

@marianoapp
Copy link
Contributor Author

Added a few changes to limit the scope of the workaround to make sure it's only active when the target vessel is loaded but not yet unpacked. Also added support for TimeWarp, both normal and physics.

@mgalyean I don't have a way to know if KSP is returning the correct position because I don't know what that should be, and the position I'm calculating is using the value reported by KSP as base. Basically what I'm doing is instead of returning "x" I'm returning "x - k".

@nuggreat It only modifies SHIP:POSITION and SHIP:DISTANCE, but part positions may also suffer from the bug, I haven't tested that.
But I get where you're going, if we only modify the ship position we could end up with parts being reported as floating in space far away from the vessel. On the other hand, I'm not sure if you can access parts positions when the vessel is still packed.

@nuggreat
Copy link

Just checked and you can access the position of parts on packed craft and they do report a different position than the ship's position.

@marianoapp
Copy link
Contributor Author

marianoapp commented Nov 27, 2020

Parts belonging to a packed vessel now return the correct position too. Since part positions are used to calculate a vessel bounds this change shifts the bounding box to the correct position as well.

@Dunbaratu
Copy link
Member

I was about to look into this last night but then I saw the chatter in the comments and realized you were about to change it some more, so I put it off.

Is it ready for merging now? I may have some time to look at it tonight.

@marianoapp
Copy link
Contributor Author

Yes, it's ready to merge

@Dunbaratu Dunbaratu merged commit 9730a13 into KSP-KOS:develop Dec 2, 2020
@marianoapp marianoapp deleted the feature/position_error branch May 25, 2021 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants