Skip to content

Commit

Permalink
fix #5079
Browse files Browse the repository at this point in the history
  • Loading branch information
rtri committed Feb 10, 2016
1 parent eb5b2b6 commit 788c804
Show file tree
Hide file tree
Showing 7 changed files with 20 additions and 27 deletions.
12 changes: 5 additions & 7 deletions rts/Lua/LuaSyncedCtrl.cpp
View file
Expand Up @@ -2371,20 +2371,18 @@ int LuaSyncedCtrl::SetUnitLandGoal(lua_State* L)
{
CUnit* unit = ParseUnit(L, __FUNCTION__, 1);

if (unit == NULL) {
if (unit == nullptr)
return 0;
}

AAirMoveType* amt = dynamic_cast<AAirMoveType*>(unit->moveType);
if (!amt) {

if (amt == nullptr)
luaL_error(L, "Not a flying unit");
}

const float3 landPos(luaL_checkfloat(L, 2), luaL_checkfloat(L, 3), luaL_checkfloat(L, 4));
const float radius = luaL_optfloat(L, 5, 0.0f);

amt->LandAt(landPos, radius);
const float radiusSq = lua_isnumber(L, 5)? Square(lua_tonumber(L, 5)): amt->landRadiusSq;

amt->LandAt(landPos, radiusSq);
return 0;
}

Expand Down
10 changes: 5 additions & 5 deletions rts/Sim/MoveTypes/AAirMoveType.cpp
View file
Expand Up @@ -66,6 +66,7 @@ AAirMoveType::AAirMoveType(CUnit* unit):
accRate = std::max(0.01f, unit->unitDef->maxAcc);
decRate = std::max(0.01f, unit->unitDef->maxDec);
altitudeRate = std::max(0.01f, unit->unitDef->verticalSpeed);
landRadiusSq = Square(BrakingDistance(maxSpeed, decRate));

useHeading = false;
}
Expand Down Expand Up @@ -138,13 +139,12 @@ void AAirMoveType::UpdateLanded()
owner->UpdateMidAndAimPos();
}

void AAirMoveType::LandAt(float3 pos, float distance)
void AAirMoveType::LandAt(float3 pos, float distanceSq)
{
if (aircraftState != AIRCRAFT_LANDING) {
if (aircraftState != AIRCRAFT_LANDING)
SetState(AIRCRAFT_LANDING);
}
const float landRadius = std::max(distance, std::max(owner->radius, 10.0f));
landRadiusSq = landRadius * landRadius;

landRadiusSq = std::max(distanceSq, Square(std::max(owner->radius, 10.0f)));
reservedLandingPos = pos;
const float3 originalPos = owner->pos;
owner->Move(reservedLandingPos, false);
Expand Down
2 changes: 1 addition & 1 deletion rts/Sim/MoveTypes/AAirMoveType.h
View file
Expand Up @@ -38,7 +38,7 @@ class AAirMoveType : public AMoveType
void SetWantedAltitude(float altitude);
void SetDefaultAltitude(float altitude);

void LandAt(float3 pos, float distance);
void LandAt(float3 pos, float distanceSq);
void UpdateLandingHeight();
void UpdateLanding();

Expand Down
12 changes: 0 additions & 12 deletions rts/Sim/MoveTypes/GroundMoveType.cpp
View file
Expand Up @@ -1415,18 +1415,6 @@ void CGroundMoveType::GetNextWayPoint()



/*
The distance the unit will move before stopping,
starting from given speed and applying maximum
brake rate.
*/
float CGroundMoveType::BrakingDistance(float speed, float rate) const
{
const float time = speed / std::max(rate, 0.001f);
const float dist = 0.5f * rate * time * time;
return dist;
}

/*
Gives the position this unit will end up at with full braking
from current velocity.
Expand Down
1 change: 0 additions & 1 deletion rts/Sim/MoveTypes/GroundMoveType.h
View file
Expand Up @@ -83,7 +83,6 @@ class CGroundMoveType : public AMoveType
bool CanGetNextWayPoint();
void ReRequestPath(bool forceRequest);

float BrakingDistance(float speed, float rate) const;
float3 Here();

void StartEngine(bool callScript);
Expand Down
2 changes: 1 addition & 1 deletion rts/Sim/MoveTypes/HoverAirMoveType.cpp
View file
Expand Up @@ -612,7 +612,7 @@ void CHoverAirMoveType::UpdateLanding()
const float distSq2D = reservedLandingPos.SqDistance2D(pos);

if (distSq2D > landRadiusSq) {
float tmpWantedHeight = wantedHeight;
const float tmpWantedHeight = wantedHeight;
SetGoal(reservedLandingPos);
wantedHeight = std::min((orgWantedHeight - wantedHeight) * distSq2D / altitude + wantedHeight, orgWantedHeight);
flyState = FLY_LANDING;
Expand Down
8 changes: 8 additions & 0 deletions rts/Sim/MoveTypes/MoveType.h
View file
Expand Up @@ -55,6 +55,14 @@ class AMoveType : public CObject
float GetMaxWantedSpeed() const { return maxWantedSpeed; }
float GetManeuverLeash() const { return maneuverLeash; }

// The distance the unit will move before stopping,
// starting from given speed and applying maximum
// brake rate.
float BrakingDistance(float speed, float rate) const {
const float time = speed / std::max(rate, 0.001f);
const float dist = 0.5f * rate * time * time;
return dist;
}
float CalcStaticTurnRadius() const;

public:
Expand Down

19 comments on commit 788c804

@ashdnazg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strafeAir have a different brakingdistance since they also have drag:
https://github.com/spring/spring/blob/develop/rts/Sim/MoveTypes/StrafeAirMoveType.cpp#L1245

@rtri
Copy link
Contributor

@rtri rtri commented on 788c804 Feb 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose the BrakingDistance function could be virtual and overriden by SAMT, but

  1. conservative estimates are better IMO
  2. 5079 affected HAMT aircraft
  3. SAMT assigns its own value to landRadiusSq: https://github.com/spring/spring/blob/develop/rts/Sim/MoveTypes/StrafeAirMoveType.cpp#L94

@ashdnazg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, forgot that!
cool then

@springjools
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes #5079 and #5080, but the solution is not optimal for fighter type planes (I guess that's SAMT), because now the Activated and StartMoving callins are not separated enough.

Before, for SAMT, the startmoving event was called when the plane is leaving ground, usually vertically, and the activated event was called when the horizontal movement begins. Many games use the activated callin to enable missiles, because otherwise SAMT can shoot when also on the ground or when just moving slightly (like when displaced because of shockwave), but the activated event makes them need a little bit of altitude before they can do that. Also used for radar or sonars to become operational with a slight delay I think.

It would be better if at least for SAMT, the activated event would still happen in Flying.

@rtri
Copy link
Contributor

@rtri rtri commented on 788c804 Feb 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Activate could be called twice (once on takeoff, once when entering the flying state) so scripts would be able to decide based on altitude, but I really prefer clean code. If you need it, a workaround is to make Activate spawn a thread that sleeps for a second or two (or whatever the expected climb-time is) and then enables weapons.

@ashdnazg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/spring/spring/blob/develop/rts/Sim/Units/Unit.cpp#L2320
activate won't be called twice without interfering with even more stuff

since we're in a feature freeze, I'd much prefer to keep it to the previous state, where activate happens on flying and deactivate on landing

@rtri
Copy link
Contributor

@rtri rtri commented on 788c804 Feb 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"could be" meaning theoretically, and activate-on-takeoff is not a feature.

I'll go along with SAMT activate on flying/deactivate after landing, but only for 101.

@ashdnazg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why if I may ask?

@springjools
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workaround would create a mismatch between activate sound event and unit animation, and if activate is called twice, it would play the sound twice.

@rtri
Copy link
Contributor

@rtri rtri commented on 788c804 Feb 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

why

because event handling should be symmetric: more predictable behavior, less chance of state conflicts. events that are each other's logical inverse (activate/deactivate) should also be coupled to logically inverse states.

a mismatch between activate sound event and unit animation

no.

if activate is called twice, it would play the sound twice.

the engine would take care of that, but it ain't happening anyway.

@ashdnazg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"events that are each other's logical inverse (activate/deactivate) should also be coupled to logically inverse states."
not to logically inverse states, but to logically inverse changes:

             Landed
        4/            \1
    Landing          Takeoff
        3\            /2
             Flying

currently activate happens on 2 and deactivate happens on 3
you changed it so activate happens on 1 and deactivate happens on 4

@ashdnazg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, a better way to put it since some state changes are bidirectional:

Before:

Deactivated

             Landed
         /            \
    Landing          Takeoff
         \            /
------------------------------ 
             Flying  

Activated

After:

Deactivated

             Landed
------------------------------ 
         /            \
    Landing          Takeoff
         \            /
             Flying  

Activated

@rtri
Copy link
Contributor

@rtri rtri commented on 788c804 Feb 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I was about to say that 2 isn't the only transition where activate happens, and 3 isn't the only transition where deactivate happens.

@ashdnazg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my point is that both decisions are logical

@rtri
Copy link
Contributor

@rtri rtri commented on 788c804 Feb 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not quite because one allows more state conflicts than the other.

@ashdnazg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how?

@rtri
Copy link
Contributor

@rtri rtri commented on 788c804 Feb 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see your own "landing on a moving carrier" example.

@springjools
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but one can also use StartMoving() and StopMoving() to get info about the landed state, minus some minor exceptions such as movement due to shockwaves. #5080 can also be worked around with a widget issuing fly state to units who are guarding, although that's probably not desirable in games with limited fuel.

Why not just check if unit has commands before calling Deactivate?

@ashdnazg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@springjools that's (more or less) how it's fixed in the latest commit

Please sign in to comment.