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.

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

Fix helicopter rotor collides at twice the rotor diameter #40313

Merged
merged 2 commits into from
May 8, 2020
Merged

Fix helicopter rotor collides at twice the rotor diameter #40313

merged 2 commits into from
May 8, 2020

Conversation

olanti-p
Copy link
Contributor

@olanti-p olanti-p commented May 7, 2020

Summary

SUMMARY: Bugfixes "Fix helicopter rotor collides at twice the rotor diameter"

Purpose of change

Fixes #39474

Describe the solution

Divide by 2

Testing

Spawned a helicopter with rotor diameter 8 and placed a wall 5 tiles away.
heli
Ascended, then descended. The helicopter lands safely with this fix but crashes without it.

@kevingranade
Copy link
Member

Round up please, integer division implicitly floors.

@olanti-p
Copy link
Contributor Author

olanti-p commented May 7, 2020

It should be floored, shouldn't it? If D=9, you count 4.5 tiles from the center of the rotor and have the same situation as in the pic

@ZhilkinSerg ZhilkinSerg added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Vehicles Vehicles, parts, mechanics & interactions labels May 8, 2020
src/vehicle_move.cpp Outdated Show resolved Hide resolved
Co-authored-by: ZhilkinSerg <ZhilkinSerg@users.noreply.github.com>
@kevingranade kevingranade merged commit b3c9415 into CleverRaven:master May 8, 2020
@olanti-p
Copy link
Contributor Author

olanti-p commented May 8, 2020

Sorry for a lack of response, I was sleeping.
Flooring was not a division error, it was intentional.

Let's say D = 2K or D = 2K + 1. Then K = floor(D/2.0). g->m.points_in_radius(R) returns an iterator over a (2R+1)x(2R+1) area. If we pass it K, we get an iterator over DxD for D = 2K + 1 and (D+1)x(D+1) for D = 2K. Example values:
D=1 ..... area=1x1
D=2 ..... area=3x3
D=3 ..... area=3x3
D=4 ..... area=5x5
D=5 ..... area=5x5

Let's say D = 2K + 1. We round it up: L = round(D/2.0) = K + 1. Then we pass L to the function and receive an iterator over (2*(K+1)+1)x(2*(K+1)+1), i.e. (D+2)x(D+2). Example values:
D=1 ..... area=3x3
D=2 ..... area=3x3
D=3 ..... area=5x5
D=4 ..... area=5x5
D=5 ..... area=7x7

For a rotor with D=5 to have a collision area of 7x7 would be unexpected for the players.

@ZhilkinSerg
Copy link
Contributor

Let's change rotor diameter to blade length maybe? It would be g->m.points_in_radius( dsp, blade_length ) (i.e. blade_length * 2 + 1) then.

@@ -419,7 +419,8 @@ bool vehicle::collision( std::vector<veh_collision> &colls,
const tripoint dsp = global_pos3() + dp + parts[p].precalc[1];
veh_collision coll = part_collision( p, dsp, just_detect, bash_floor );
if( coll.type == veh_coll_nothing && info.rotor_diameter() > 0 ) {
for( const tripoint &rotor_point : g->m.points_in_radius( dsp, info.rotor_diameter() ) ) {
size_t radius = static_cast<size_t>( std::round( info.rotor_diameter() / 2.0f ) );
Copy link
Contributor

@jbytheway jbytheway May 8, 2020

Choose a reason for hiding this comment

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

If division by two really is intended, suggest using divide_round_up rather than floating-point arithmetic here.

@olanti-p
Copy link
Contributor Author

olanti-p commented May 8, 2020

Sounds good to me. The lift thrust calculation can be easily adapted to use blade length too. I can make it a PR. Does it include changing diameter to blade length in json too?

@kevingranade
Copy link
Member

Flooring was not a division error, it was intentional.

I understand, I just disagree.
When rotor diameter exceeds 1m (it always will), it necessarally enters tiles adjacent to it's origin. In order for that to happen when D = 1.1, R = f( D ) must be greater than 1.
From there, as D increases, R obviously increases at half the rate of D.
Given these invariants, the most straightforward definition of R is ceil( D / 2 ).

From a balance point of view, anything we do here is incredibly generous, from what I've read about helipad layout, minimum safe landing zone footprint is 2D. It is very likely that we will add up to several tiles of horizontal jitter when ascending and descending to represent this. Because of this, I'm not particularly concerned about the, "players will be surprised that they can't land in a tiny area" issue, because that is a fundamentally unsafe thing to do. If you want to alleviate the surprise factor there, we can look into emitting a, "minimum safe distance" number, but that number should be significantly larger than the physical rotor footprint.

@olanti-p
Copy link
Contributor Author

olanti-p commented May 8, 2020

So it basically adds 0.5-1 tile to real radius as a penalty for tiny jitter? Can't argue that. Should be mentioned somewhere in the code probably.

If you want to alleviate the surprise factor there

No, it's fine. It was more of a "this code produces wrong R for given D since helicopters are absolutely still" issue. When (if) the jitter is added, the surprise factor will no longer be there since helicopters would be visibly affected by jitter/wind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Vehicles Vehicles, parts, mechanics & interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helicopter rotor collides at twice the rotor diameter
4 participants