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 Hovers defaults as well as some potential crashes #16496

Merged
merged 4 commits into from Jun 2, 2019

Conversation

@reaperrr
Copy link
Contributor

commented May 4, 2019

Fixes #16336.

Details are in the commit descriptions.

@reaperrr reaperrr added this to the Next Release milestone May 4, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented May 4, 2019

We normally prefer to throw an exception at load-time (which also includes the linter) vs sanity-fixing bogus values at runtime. Can we instead implement RulesetLoaded and throw a YamlException if the values are bogus?

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Can we instead implement RulesetLoaded and throw a YamlException if the values are bogus?

The problem is that without the clamping, some of the bugs trigger even on values that don't look suspicious at all at first glance, because the diff between InitialHeight and OffsetModifier plays into that, too (in addition to the tick values).
Calculating on which values to throw would be a real pain, and I currently don't have the brain bandwidth and motivation to properly debug/fix/refactor the math of this trait, either.

All in all and considering how Hover Tanks worked in original TS, I'm beginning to think this trait isn't really suited to alter the visual 'center position' of hovering actors.
Things like falling to ground on EMP/Ion storm, sinking if that happens over water, and the twirling during fall all would be much easier to do if hover units actually changed their real altitude, via Mobile/Locomotor.
In the long run I think this trait should only handle the slight bobbing up and down, like it used to.

That said, unless you feel like taking over and reviewing + fixing the traits' math properly, it's basically 'take as-is' (except for minor fixes, if necessary) or revert the PR that introduced those issues, though that would also lose us some functionality again.

@pchote

This comment has been minimized.

Copy link
Member

commented May 5, 2019

Ok, makes sense and I agree. Will re-review with a new focus on this as a stop-gap solution to improve the current behaviour until a future PR can move the global hover height/disabling to Mobile / Locomotor.

@pchote

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Can we please at least check:

Must be a negative value!

Must be higher than zero.

Must be at least as high as RiseTicks!

Using RulesetLoaded? We can then omit them from the descriptions.

@reaperrr reaperrr force-pushed the reaperrr:fix-Hovers branch 2 times, most recently from 2fe7c3a to 0e0213a May 11, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

Can we please at least check (...) using RulesetLoaded? We can then omit them from the descriptions.

Done. That actually also allowed to remove all but one clamp, so it's pretty close to your original request.

@pchote
Copy link
Member

left a comment

LGTM. Just my comment above about OffsetModifier, which still stands.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

Updated.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

until a future PR can move the global hover height/disabling to Mobile / Locomotor

Do we want / have an issue for this?

@matjaeck
Copy link
Contributor

left a comment

LGTM

@abcdefg30
Copy link
Member

left a comment

You are changing a bunch of default values as it looks. Are we fine with not having an update rule? (I actually assume yes as you labelled those default changes as bugfixes.)

this.fallTickHeight = (info.InitialHeight + info.OffsetModifier) / info.FallTicks;
stepPercentage = 256 / info.Ticks;

// fallTickHeight must be at least 2 to avoid a DivideByZeroException and other potential problems when trait is disabled.

This comment has been minimized.

Copy link
@abcdefg30

abcdefg30 May 31, 2019

Member

Can you explain this? I don't understand how we will get a divide by zero if this value is 1.
var fallTicks = worldVisualOffset.Z / fallTickHeight - 1; should divide first, then subtract one.

This comment has been minimized.

Copy link
@reaperrr

reaperrr May 31, 2019

Author Contributor

Right, but the value still needs to be at least 1 to avoid said DBZ Exception. Updated accordingly.

@abcdefg30

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Need a rebase, too.

@pchote

This comment has been minimized.

Copy link
Member

commented May 31, 2019

You are changing a bunch of default values as it looks.

These are mostly restoring the original values from an earlier PR that did not provide an update rule.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

Rebased & updated.

Updated

@abcdefg30
Copy link
Member

left a comment

Orcas in TD still bob differently compared to the last release.

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

The only way to "fix" that is to revert #16261, which fundamentally changed the bob calculation.

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

I think increasing the speed of the bobbing will make it looks similar to the release again, for example halfing to Ticks: 6.

reaperrr added 4 commits May 4, 2019
Polish various aspects of Hovers
- Use WDist instead of int for fields
- Change default values to approximately restore previous
  releases' default behavior
- Improve and clarify descriptions
Fix or prevent bugs in Hovers
- Clamp fallTickHeight to at least 1,
  to avoid potential DivideByZero crash.
- Prevent modders from setting values that
  are bogus or would trigger other bugs,
  via loadtime YamlExceptions.

@reaperrr reaperrr force-pushed the reaperrr:fix-Hovers branch from b0e32f5 to 2ea2d76 Jun 1, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

for example halfing to Ticks: 6.

Done.

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

Imho this PR has changed a bit since the initial reviews, so waiting for someone else to reconfirm their 👍 by looking if the new bobbing speeds look acceptable.

@pchote
pchote approved these changes Jun 2, 2019

@pchote pchote merged commit 3ff8fb3 into OpenRA:bleed Jun 2, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.