-
-
Notifications
You must be signed in to change notification settings - Fork 385
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
Docking with orbital in manual mode #5619
Docking with orbital in manual mode #5619
Conversation
I've recorded a short showcase of this feature: Manual.Docking-1.mp4 |
DOCK_ANIMATION_NONE, | ||
DOCK_ANIMATION_1, | ||
DOCK_ANIMATION_2, | ||
DOCK_ANIMATION_3, | ||
DOCK_ANIMATION_MAX, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered replacing the hard-coded 4-stage docking animation to instead be a dynamic list of Tags generated from the model file setup? I'd expect that to be significantly more flexible for future changes to the docking procedure (e.g. ships "flying themselves" along the docking path) and also allow e.g. using a "locator beacon" well outside the station for the first approach node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, the animation of a specific station has an arbitrary number of stages, for example, ground stations have none at all. And it is read from the tags in the model, although there is some hard-coded logic that adds intermediate "locators". This is old code and it works well with current models.
I think it will be easy enough to turn this logic into a separate dynamic list when needed.
External beacons are stored separately in DockStage::APPROACH1
and DockStage::APPROACH2
. I think that conceptually they should be located separately, so that, for example, they do not rotate with the station.
Apologies I've not touched this for some time - I somehow was under the mistaken impression it was still WIP. If you could rebase against master (just to make sure it works with the physics changes I merged recently), I'll go ahead and do a full review the next time I get a chance. |
Were previously added a separate type of collision meshes to the orbital, flagged "ENTRANCE". These surfaces are identified by their name beginning with "collision_port". Now, upon collision with it, if player (or any other ship) have autopilot turned on, docking occurs, if not, nothing happens. This is necessary so that the player can, on the one hand, fly into the station for manual docking, on the other hand, NPC ships and autopilot can use the old docking mechanism.
b2763b6
to
48af70f
Compare
Rebased, tested landings/takeoffs - it seems to still be working! |
Here are the updated collision meshes. Pads aren't overlapping with the volume and port meshes. |
for(auto &g : m_dynGeoms) { | ||
g->Enable(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious if this solves the recent batch of issue reports regarding inability to collide with / land on stations. I did make the CollisionSpace code stricter, as it no longer includes disabled Geoms in the BVH construction at all (rather than filtering them out later in the pipeline).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added these changes to the master and tested #5705 - does not help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments where I see some stylistic changes that could be made to improve the readability of new code.
I will test this PR sometime in the next few days and if no major issues arise (and once the review comments are addressed) I'll rubber-stamp this for merge.
I will abstain from a more in-depth review - most of the changes I'd request would involve rewriting large parts of how SpaceStation is designed and architected.
In that context, there's less need to hold this code to a minimum standard of maintainability. Instead, I consider this PR to be a substantive improvement to the current player experience regarding space stations and will approve it based on that merit.
Just a reminder to not smash merge button until FIXUP commits are rebased. Regards, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approval-ed, pending rebase to appease the resident git-fascist.
Great! Need to wait for a little more improvement in the collision meshes, related post - #5619 (comment), and I will make a rebase. |
@Gliese852 These should be good: |
If the autopilot is not enabled, instead of tossing the ship from the pad at full throttle, we turn on the upward cruise at a speed of 2 m/s and the ship rises smoothly and beautifully.
In fact, this is a quite large refactoring of SpaceStation and SpaceStationType, as a result of which it becomes trivial to implement manual docking. Made a step towards more consistent terminology: bay - means one specific seat for the ship port - this is a group of bays united by a common entrance, exit, size Although I could have missed something - port sometimes means bay. Also some inline small fixes / improvements - disengage autopilot during docking rails - add easing, which is useful in docking rails - correct rarely used matrix4x4 functions Right, Up, Back (at least it will be consistent with the VectorX, VectorY, VectorZ functions in matrix3x3, and correctly applied to matrices of imported tags) - disable geoms when colliding is turned off
- remove bulging collision meshes: Collision meshes were slightly bulging from their respective tags, resulting in a collision event when manually undocked from the orbital. - don't overlap the entrance and pad meshes: Otherwise, sometimes collisions between the ship and the pad might not occur - only the first 8 collisions encountered are processed, and these were collisions with the entrance collision mesh. They are simply discarded. Models have been edited by Nozmajner.
eaa7fb0
to
848c856
Compare
Rebased fixup commits, ready to merge. |
…game start No longer required since pioneerspacesim#5619 was merged
…game start No longer required since pioneerspacesim#5619 was merged
…game start No longer required since pioneerspacesim#5619 was merged
…game start No longer required since pioneerspacesim#5619 was merged
…game start No longer required since pioneerspacesim#5619 was merged
…game start No longer required since pioneerspacesim#5619 was merged
If you fly in or launch from the orbital station on autopilot - everything works as before, I just improved these animations a little. But if your autopilot is off, you can fly into the station and land manually.
Since the station is rotating, it is recommended to turn on the "Follow orientation" mode, then the ship will compensate for the rotation of the station. Although, you can land completely manually.
In order not to hit anything, when manually starting from the orbital, the "cruise-up" mode is automatically turned on. It seemed to me that it works well in other places too - when launching from a ground station or from the surface - you are slowly ascending at a speed of 2 m/s.
In fact, this is a quite large refactoring of SpaceStation and SpaceStationType, as a result of which it becomes trivial to implement manual docking.