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 floating-point precision problem in apply_light_arc #76959

Merged
merged 2 commits into from
Oct 14, 2024

Conversation

SurFlurer
Copy link
Contributor

@SurFlurer SurFlurer commented Oct 11, 2024

Summary

Bugfixes "Fix floating-point precision problem in apply_light_arc"

Purpose of change

Fix #76909

Describe the solution

Instead of using std::floor to determine which octant the light beam should fall into, I'd rather compare the boundaries of light beam with octant boundaries to make sure nothing is incorrectly rounded.

This also simplified the code by using a unified approach to cast light into one octant.

Describe alternatives you've considered

Fix the rounding problem of units::angle : Would love to leave that to more authoritative ones to decide.

Testing

  1. Install a headlight pointing at 45° from straight forward (installing, then pressing up and then left) and drive around, making sure the light beam looks fine.
  2. Install a headlight pointing at 27° from straight forward (installing, then pressing up x2 and then left) and drive around, making sure the light beam looks fine.
  3. Install a wide angle headlight pointing at 45° from straight forward (installing, then pressing up and then left) and drive around, making sure the light beam looks fine.

Additional context

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Oct 11, 2024
@PatrikLundell
Copy link
Contributor

  • What happens to oangle when angle is 0 (I don't know what fmod does)? Is it normalized to a positive angle or do we end up with a negative angle? It looks like the code expects the start angle to be lower than the stop angle and that both angles are to be positive. That's only going to be true if you add 360 degrees to both angles when the start angle is negative initially (and the modulo 8 in the switch statement would handle the extension beyond a full circle, so that part is taken care of).
    Angles are messy because their circular nature means crossing over the 0 degree boundary requires handling.
  • Is there a guarantee the original angle is non negative, or does it have to be normalized to ensure it is?
  • The code doesn't actually handle floating point inaccuracies, but still relies on comparisons of start/stop angles to 45 degree multiples. This will fail for 45 degree angle multiples when a start angle ends up slightly less than the comparison angle or the stop angle being slightly larger than the 45 degree multiple comparison angle. If you go the code route you'll have to introduce a delta constant to nudge the comparisons to the safe side.
    If the code works it's just luck (and may be processor dependent if the underlying architecture uses different sized floating point calculations).

@SurFlurer
Copy link
Contributor Author

SurFlurer commented Oct 11, 2024

What happens to oangle when angle is 0

Thanks for reminding. I actually intended to have a normalized oangle between 0_degrees and 360_degrees. But I forgot to check the behavior of fmod. Fix is on the way.

Is there a guarantee the original angle is non negative, or does it have to be normalized to ensure it is?

It has to be normalized.

The code doesn't actually handle floating point inaccuracies, but still relies on comparisons of start/stop angles to 45 degree multiples. This will fail for 45 degree angle multiples when a start angle ends up slightly less than the comparison angle or the stop angle being slightly larger than the 45 degree multiple comparison angle.

My rationale is to make floating point inaccuracy unimportant. oangle, with its immutable nature, can only fall into a octant once. cangle too. Therefore, even if they fall into adjacent octant due to inaccuracy, they still should be fairly close to octant boundaries. For example, if oangle is 44.9_degrees and cangle is 90.1_degrees, surely castLight will be called 3 times, for 44.9-45 degrees, then 45-90 degrees, and then 90-90.1 degrees respectively. But this is a fairly good approximation to the intended 45-90 degrees light beam.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Oct 11, 2024
@PatrikLundell
Copy link
Contributor

Ah, so you're saying the castlight calls are actually doing something useful with almost correct parameter values and that this doesn't gets degraded by the inherent blockiness of the result (as it has to be tiles being illuminated). Such as illuminating a tile to 99.999% of what it should have been, and another by 0.99999% when it shouldn't be?

@SurFlurer
Copy link
Contributor Author

Ah, so you're saying the castlight calls are actually doing something useful with almost correct parameter values and that this doesn't gets degraded by the inherent blockiness of the result (as it has to be tiles being illuminated). Such as illuminating a tile to 99.999% of what it should have been, and another by 0.99999% when it shouldn't be?

I'm not sure I fully get what you mean, but from my understanding, although slightly inaccurate parameter values can possibly lead to the boundaries of adjacent octants being falsely illuminated, in fact the octants' boundaries are overlapping, so the falsely illuminated tiles of one octant are effectively correctly illuminated tiles of another. On the other hand, castLight treats start and end ledges pretty permissively, so start = 1.0F functionally equals to start = 0.99999F if the reality bubble doesn't extend too much. Therefore I don't even believe it would make the light cone slightly wider or narrower.

@Maleclypse Maleclypse merged commit 2e5b3cf into CleverRaven:master Oct 14, 2024
25 of 26 checks passed
@SurFlurer SurFlurer deleted the fix-light-arc branch October 14, 2024 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Headlight beam is being blocked with no apparent cause, depends on vehicle orientation
3 participants