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.

Sign up for GitHub

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

Jump to bottom

Incorrect logic in Ship::Move breaks Command::STOP #29

Open
1 task done
UnorderedSigh opened this issue Jun 28, 2023 · 6 comments · May be fixed by #32
Open
1 task done

Incorrect logic in Ship::Move breaks Command::STOP #29

UnorderedSigh opened this issue Jun 28, 2023 · 6 comments · May be fixed by #32
Labels
bug Something isn't working

Comments

@UnorderedSigh
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

When the AI decides to stop the ship, sometimes it can't. This happens due to an error in the new thrusting logic. Sometimes, the code will ignore dragAcceleration, which is the variable Ship::Move uses to implement Command::STOP when the ship is almost stopped. This prevents ships from boarding, landing, assisting, and a variety of other things, if they happen to be moving at just the right speed. That's why the integration tests are failing.

Steps to Reproduce

Run the integration tests a few times on Ubuntu 20 or 22 using GL. Watch Capture Uncapturable With Capturable Override fail.

Expected Behavior

Ships can stop. All integration tests should pass.

Screenshots

No response

Link to save file

No response

Operating System

Ubuntu 22

Game Source

Built from source

Game Version

e1004d1

Additional Information

This is the fix:

diff --git a/source/Ship.cpp b/source/Ship.cpp
index 61d4b668c..47fa1f485 100644
--- a/source/Ship.cpp
+++ b/source/Ship.cpp
@@ -2249,7 +2249,7 @@ void Ship::Move(vector<Visual> &visuals, list<shared_ptr<Flotsam>> &flotsam)
                                if((aNormal > 0.) != (vNormal > 0.) && fabs(aNormal) > fabs(vNormal))
                                        dragAcceleration = -vNormal * angle.Unit();
                        }
-                       if(velocity.Length() > MaxVelocity() || velocity.Length() < 0.1)
+                       if(commands.Has(Command::STOP) || velocity.Length() > MaxVelocity() || velocity.Length() < 0.1)
                                velocity += dragAcceleration;
                        else
                                velocity += acceleration;

I don't want to do a PR until the conflicts between master and experimental are resolved.
@UnorderedSigh UnorderedSigh added the bug Something isn't working label Jun 28, 2023
@UnorderedSigh UnorderedSigh mentioned this issue Jun 28, 2023
Merged
@UnorderedSigh
Copy link
Author

I have retested experimental with this fix, and it doesn't fix the problem. I'm still investigating.

@UnorderedSigh UnorderedSigh linked a pull request Jul 6, 2023 that will close this issue
Draft
@UnorderedSigh
Copy link
Author

I have a fix for this, but a later test fails now.

@Zitchas
Copy link
Member

Zitchas commented Mar 24, 2024

It's been a while and I'm just getting back up to speed. Is this bug still a thing?

@xobes
Copy link

xobes commented Nov 7, 2024 •
edited
Loading

  1. I came to this thread to ask if the down-arrow to turn around and allow me to press forward to slow down is intentionally broken in this fork/branch (which is it, anyway)
  • also -- actually, when i press the down arrow at a low enough speed i start going backwards in my star barge -- no reverse thrusters, so that's not right.
  1. I see up there something about integration tests, but on my linux box I just pulled this down and built the Debug build but could not do ctest --preset linux-integration, as it claimed No tests were found!!!

@TheGiraffe3
Copy link

  1. I came to this thread to ask if the down-arrow to turn around and allow me to press forward to slow down is intentionally broken in this fork/branch (which is it, anyway)
  • also -- actually, when i press the down arrow at a low enough speed i start going backwards in my star barge -- no reverse thrusters, so that's not right.

In Delta, all thrusters also have some reverse thrust. So down to turn around intentionally doesn't do anything, and reverse thrusters are just an extra bonus - you don't actually need them to go backwards.

@Zitchas
Copy link
Member

Zitchas commented Nov 7, 2024

Thanks for the feedback!

  1. By default, the down arrow switches to reverse thrust when the ship has some reverse capability; and currently all ships have limited amounts of reverse and lateral thrust. That being said, having a control to tell your ship to flip around to exactly counter your current movement is a useful command, so we should probably figure out a way to force that behavior. Something tickles the back of my mind that it might already exist, but I'm not sure about that.
  2. Unfortunately, my knowledge of the tests or how to invoke them is pretty limited. On Visual Studio on Windows the tests don't run on a normal or debug build; I have to trigger them by telling VS to do the tests. I would suspect that this is also the case on Linux.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix movement bugs that break integration tests UnorderedSigh/endless-sky
5 participants
@xobes @Zitchas @UnorderedSigh @TheGiraffe3 and others