scp-fs2open / fs2open.github.com

Origin Repository for SCP FreeSpace 2 Open
https://www.hard-light.net/
Other
407 stars 164 forks source link

ship-maneuver changes cause ludicrous speed #2157

Closed MjnMixael closed 1 year ago

MjnMixael commented 4 years ago

BtA's mission Rebels and Revolutionaries displays this problem.

Oct 24 build, the Seeker ships fly at a normal speed (about 20 m/s). This is done with a ship-maneuver sexp to tell the ship to fly forward at 100% velocity while rotating slightly.

After Oct 26 changes submitted by @Goober5000 the Seeker ships fly off into the sunset at +160 m/s.

Goober5000 commented 4 years ago

Screenshot... https://cdn.discordapp.com/attachments/223511363807346691/648545575381237781/screen0037.png

Goober5000 commented 4 years ago

I tried the most recent build from source, and that worked perfectly fine. Next I tried the 20191112 build, and that worked too. Finally I tried the 20191026 build, and that also worked. In all three builds, both Seeker 1 and Seeker 2 went 20 m/s as expected. I confirmed that I was running the proper builds by checking the build ID string in the main hall.

Goober5000 commented 4 years ago

Is this still a problem?

MjnMixael commented 4 years ago

Unsure. It has been on my list to look in to again and I just haven't.

z64555 commented 3 years ago

Is the rotation on any one axis? Bank, perhaps?

JohnAFernandez commented 3 years ago

We don't have reproduction and MJN has too much on his plate in RL to really look at this. I don't think we can easily fix this right now in the next few weeks.

But if anyone is looking into it, I would suggest looking at the fact that the object was docked to 30 other objects and not just looking at the sexp. I'm not sure if @Baezon's changes to the moment of inertia for docked objects may have inadvertently fixed this as well, because notice that the sexp was slightly turning the object.

But like I said, I just don't think we can fix this without being able to reproduce it, and it may be a while before MJN or someone else can give us a good handle on how to do that.

Baezon commented 3 years ago

I'm with Cyborg here, without a reliable means to produce it there's not much we can do. I don't feel confident enough to close it, but I don't think it's immediately urgent enough to warrant a high priority for the stable.

z64555 commented 3 years ago

Alright, bumping it down to Low for now

JohnAFernandez commented 3 years ago

BTA team has not seem to encountered it recently. Closing until there is a new report.

MjnMixael commented 2 years ago

This showed back up again for a player. See here.

I took another look at the mission. I noticed two things. ship-maneuver has an optional 11th argument now that is a bitfield. Two things that can be turned on with that argument caught my eye. One allows the sexp to move the ship greater than tabled' values and the other keeps old ship-maneuver values.

The other thing I noticed was that my Events that trigger the ship-maneuver were repeat firing (had a -1 trigger count and a 3 second interval). This should not cause ship-maneuvers to build up.. but these are the only ship-maneuver sexps in the mission and the ships themselves have no waypoints. Their movement is entirely controlled by these Events.

So, with all that in mind, I would suggest looking in two places for any obvious errors. First, check that the optional argument defaults to all options being off. My Events to not specify the optional arguments because the mission was made before those existed and this issue started after they were added in PR #2118 according to my original note. I would also check that if those defaults aren't OFF if not specified, then make sure that the bit that allows maneuvers on top of maneuvers is working as intended.

JohnAFernandez commented 2 years ago

That's good detective work. If I have some time, I'll try to look at it this weekend, checking for exactly what you're talking about.

JohnAFernandez commented 2 years ago

I had some time tonight. The initial variable that takes the 11th argument seems to be properly zeroed, but maybe there's a problem if the 11th argument is not specified. I'm going to keep digging tomorrow night.

Goober5000 commented 2 years ago

To follow up on this, the optional 11th argument will default to the options being off if they are not specified.

However, the bit that adds maneuvers on top of maneuvers may indeed be the culprit. In the function sexp_set_ship_man, the CIF_DONT_OVERRIDE_OLD_MANEUVERS check will only reset the flags themselves. The rest of the values (rotation and translation) are not zeroed out.

This flag was originally added by @Baezon so I'll ask for his input.

jg18 commented 2 years ago

I created a test mission based on the BtA mission in question, but I was unable to repro.

I don't think there's anything more we can do about this until we have a reliable repro (sorry Mjn).

To make testing easier, I created a "modpack" VP with all the non-retail assets needed to run the mission. To get GH to accept it as an attachment, I put a .7z inside a .zip. test_2157.7z..zip

jg18 commented 2 years ago

Given my previous comment about being unable to repro this, closing it for now.

We can always re-open this issue if/when someone can repro it, for which the test mission/"modpack" in the last comment might help.

jg18 commented 2 years ago

Re-opening per SCP standard procedures, although the ticket status is effectively unchanged.

JohnAFernandez commented 2 years ago

So, I investigated some more, and I think there are some important facts that need to be mentioned.

  1. The freighter that is moving at excess speeds is docked with many pods that are spitting out fighters.
  2. None of the other ships in the arrangement have maneuver sexps or invulnerability of any kind.
  3. The freighter is not getting its forward thrust from the maneuver sexps but from it trying to complete its waypoints.
  4. Its speed is not a constant acceleration. At one point, it slows down and then speeds right back up. See the screenshots from the one time the bug was caught on camera. Screenshot 1, at 0 seconds its speed seems to be at least 160. Screenshot 2, at 2 seconds, it drops to below 100, and then, screenshot 3 less than a second later, it picks right back up. If the AI were behaving like normal, just with its max speed flag set, then it wouldn't have such wild swings in speed. It is very likely that the instantaneous acceleration flag is set as well. It is impossible to tell if the don't bank when turning flag is set.

Screen Shot 2022-06-11 at 2 53 43 AM Screen Shot 2022-06-11 at 2 53 40 AM Screen Shot 2022-06-11 at 2 58 32 AM

If the acceleration were caused by some weird collision bug, then it wouldn't be likely to start and stop, It also wouldn't get faster over time, but would reach some equilibrium speed, and the hud gauge would flash.

The real question is, what value is the flag argument set to if it is not specified. Because if it's set to -1, then because of signed ints, it would make all flags true.

Baezon commented 2 years ago

In sexp_set_ship_maneuver() maneuver flags is properly initialized to 0.

JohnAFernandez commented 2 years ago

I realize this, but it's getting its value from maneuver_flags = eval_num(n, is_nan, is_nan_forever); after. So the question remains, what is the value of the node when it is not specified in the mission file.

Baezon commented 2 years ago

If the node doesn't exist then the (n >= 0) check won't be true, and eval_num() will be skipped, leaving it as whatever value it was before.

JohnAFernandez commented 2 years ago

So we need to make sure the node doesn't exist, because there is literally no other explanation. As you said, maneuver_flags initialized to zero, and there's no other way in the codebase for that flag to be set, since they weren't using scripts.

Baezon commented 2 years ago

Being a bug, I assure you it's entirely possible there are several other explanations. By their very nature they are unpredictable, it may not even be related to the maneuver flags at all, that's just a theory. It could be any number of other things related to the AI or the physics, things which are also important elements of this situation. It could even be some sort of memory corruption, who knows. Auditing the maneuver code is good, but it's not the only possible culprit.

EatThePath commented 2 years ago

In investigating A FCW related bug with ship-maneuver that may in fact be the same bug as this, my finding has been that... ship-maneuver sets some ai_override_flags ai_control_info_check() in aicode.cpp sets some physics info flags in response to the ai_override_flags if the ship is under AI control. I haven't hard confirmed the sequence of events because I don't know how this sexp works or how any of the related physics, ai or controls code works, but it appears to me that if the AI control ends before the ship-maneuver duration ends then some flags will be left in the physics info flags.

z64555 commented 2 years ago

Punting to 22.4 due to this issue's fickle nature.

MjnMixael commented 1 year ago

It's been six months since the merge of #4483 and I have not seen or heard of this bug rearing up again in BtA. It seems likely that fixed whatever was causing the problem.

JohnAFernandez commented 1 year ago

We can always reopen if there is another report. Thanks for confirming!

Goober5000 commented 1 year ago

Ironically I may have encountered this bug just last night. I discussed it briefly with @Baezon and we will try poking at it a bit more.

The symptom I saw was that one ship-maneuver sexp was causing multiple speed changes. It did not accelerate to ludicrous speed, but it did increase past the speed limit it was given.

Goober5000 commented 1 year ago

@Baezon discovered the cause of the acceleration and it was a different bug, likely not related to this. So I'll close this again.