View Issue Details

IDProjectCategoryView StatusLast Update
0001050FSSCPphysicspublic2006-09-23 11:05
ReporterBackslash Assigned ToBackslash  
PrioritynormalSeveritytweakReproducibilityalways
Status resolvedResolutionfixed 
Summary0001050: max_speed is a magnitude of Max Velocity, causing a trichord bug
DescriptionIn ship.cpp we have the lines:
if(optional_string("$Max Velocity:"))
   stuff_vector(&sip->max_vel);

// calculate the max speed from max_velocity
sip->max_speed = vm_vec_mag(&sip->max_vel);

The problem we noticed is, what if Max Velocity consists of more than just forward, so sliding speeds as well? max_speed then becomes MORE than the max Z speed velocity, and thus this new value becomes your maximum throttle. (Note the maximum throttle is only applied to the Z axis.)

$Max Velocity: 0, 0, 50 shows your maximum forward speed as 50.
$Max Velocity: 50, 0, 0 STILL shows your maximum forward speed as 50, even though you have specified 0. (sqrt(50^2+0+0))
$Max Velocity: 50, 50, 50 shows your maximum forward speed as 86 (sqrt(50^3)). Worse, during trichording you can go up to 112! (sqrt(50^2+50^2+86^2) (Of course, this is the _real_ speed you're going... the in-game speed will just show your Z velocity. You might need the Velocity Indicator script (http://www.hard-light.net/wiki/index.php/Script_-_Velocity_Indicator), or custom debug code in the throttle indicator.
Additional InformationI posted this in
http://www.hard-light.net/forums/index.php/topic,41832.0.html a couple weeks ago.
Goober mentioned this was related to an attempt to deal with the trichording bug... however in my opinion this code CAUSES a trichording 'bug'.

I'd change this myself right now but I suspect there's some issue I might not be considering, so thought I'd ask about this first. Is there ever a situation where max_speed SHOULD be taking sliding into account? Maybe Mantis will get a bit more attention, since this turns out to manifest as a bug. and hey, this way if I end up fixing it, it's recorded in Mantis as something I fixed :-D
TagsNo tags attached.

Activities

Goober5000

2006-09-15 21:03

administrator   ~0006634

You know, I'm glad you brought this up. I didn't realize it before, but this is the same thing argv noticed three years ago. I saved his email because I wasn't sure what to think of it, but perhaps he was right after all:

-----------------------------------------------------------------

In code/ship/ship.cpp, parse_ship(), we have:

    sip->max_speed = vm_vec_mag(&sip->max_vel);

Goober comments:

    // Goober5000 - it's always worked before, and suppose a ship moves in
    // multiple dimensions?

Ironically enough, this breaks if the ship does move in multiple dimensions.

The max_speed field is used only for the max forward speed; max speed in other
dimensions and backwards is done elsewhere. Doing a vm_vec_mag here is likely
to give a max_speed that is higher than the ship's max oclk speed, in which
case increasing the ETS engine power causes max speed to decrease!

The code I put here is:

    sip->max_speed = sip->max_vel.xyz.z;

I've tested this with both ships that move in multiple dimensions and those
that do not; this gives correct results in both cases.

Try it yourself if you don't believe me: make a player ship that can move in
multiple dimensions, and a max oclk speed higher than the normal max speed.
Play it, and increase the engine power on your ETS to full. Notice how the
resulting max speed (the one displayed on your HUD speed gauge) is not the
max oclk speed you set in the table, and is probably less than the max speed
you got with equal ETS settings.

For this reason, I am putting my fix back in for my experimental branch. It
may go into HEAD pending resolution of this issue.

_argv[-1].

Backslash

2006-09-15 22:44

developer   ~0006635

Three years? Wow. So _argv[-1] was a human! ;-P What else did he do?

Yeah, that small code change seems to work so far. I'm not noticing any problems, and I'm testing the more complicated things that use max_speed, like autopilot. I'll commit this to HEAD on Sunday if nobody comes up with an objection.

Heh, that makes me think... does the AI know how to use craft with zero maximum Z speed and non-zero maximum X or Y speed? This could be funny :-D

Goober5000

2006-09-15 23:28

administrator   ~0006636

Why don't you go ahead and test it? ;)

I'd like to hear a go/no-go from taylor, but at this point it looks like this fix should go in.

taylor

2006-09-16 00:20

administrator   ~0006637

I've seen the ETS problem before, but never really looked into it. Nice to finally know what that was. :)

I don't have a problem with it going in either branch (assuming that it's well tested before it hits the stable branch at least) since I feel that this will be a rather significant problem with the upcoming mods which make use of this (which is most of them).

My only concern is that there almost appears to be more to this than we're seeing. How AI handles this will be something to watch out for since I have a strange feeling that this could do some weird things to balance with Shivan ships in the original campaigns. This is just subtle enough to break balance in a few cases. I'm not sure how to test that theory out though.

Backslash

2006-09-16 00:53

developer   ~0006638

Exactly why I brought this up, the subtle effects I don't know about.

Heh, and in that case you're going to LOVE my next report :-P

Backslash

2006-09-20 08:22

developer   ~0006656

Heh, maybe I should go look at what else argv was doing.

Committed. I'll commit it to 3_6_9 in a few more days (and once I figure out how :-P )

taylor

2006-09-21 12:24

administrator   ~0006661

Ugh, I reverted your commit. You had a single change, but that took a 122k, 3,921 line diff to make happen. Please get those excess commit changes in check and re-commit the fix.

I'd seriously hate to keep reverting your stuff because of CVS client settings. If you aren't sure what it's going to do then get a diff before you commit and review what it intends to change first. If it's not what you expect, then make the needed changes, and get another diff to verify that it's correct before final commit.

Backslash

2006-09-22 08:00

developer   ~0006667

Aiee! I apologize, that IS seriously annoying! By all means, revert, and thank you.

Thing is, I DID do a diff before committing and it showed me what I expected to see, so I thought I had the problem licked. But now that I look back it seems TortoiseCVS ignores whitespace diff.

Bleah... I liked WinCVS better (hahaha!) but it refuses to commit...
[sizable gap of time]
Ok, after a reinstall, recheckout, and barking up many of the wrong 'trees' (ba-dum tsh), I seem to have fixed the problem (with WinCVS, and thus hopefully the whitespace problems). Shouldn't happen again. Slap me up if it does.

Backslash

2006-09-23 11:05

developer   ~0006682

Fixered, really this time.

Other than how much CPU it takes up even idle, I much prefer the amount of advanced options WinCVS now gives me, especially regarding forcing Unix file format, and whitespace.

Issue History

Date Modified Username Field Change
2006-09-15 20:08 Backslash New Issue
2006-09-15 21:03 Goober5000 Note Added: 0006634
2006-09-15 22:44 Backslash Note Added: 0006635
2006-09-15 23:28 Goober5000 Note Added: 0006636
2006-09-16 00:20 taylor Note Added: 0006637
2006-09-16 00:53 Backslash Note Added: 0006638
2006-09-16 23:29 Backslash Status new => assigned
2006-09-16 23:29 Backslash Assigned To => Backslash
2006-09-20 08:22 Backslash Status assigned => resolved
2006-09-20 08:22 Backslash Resolution open => fixed
2006-09-20 08:22 Backslash Note Added: 0006656
2006-09-21 12:24 taylor Status resolved => feedback
2006-09-21 12:24 taylor Resolution fixed => reopened
2006-09-21 12:24 taylor Note Added: 0006661
2006-09-22 08:00 Backslash Note Added: 0006667
2006-09-23 11:05 Backslash Status feedback => resolved
2006-09-23 11:05 Backslash Resolution reopened => fixed
2006-09-23 11:05 Backslash Note Added: 0006682