Skip to content

[gpad] avoid potential float precision lost in PaintHatches #18510

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

Merged
merged 1 commit into from
Apr 25, 2025

Conversation

linev
Copy link
Member

@linev linev commented Apr 25, 2025

Use integer conversion once and then just check boundary with ymin

Alternative to #18494

Use integer conversion once and then just check boundary with ymin
@ferdymercury
Copy link
Contributor

ferdymercury commented Apr 25, 2025

Side note: an integer for loop is faster than doing a double comparison on every loop (while) step for stopping. Can be optimized better by compiler, too. That's why I suggested precalculating the number of steps. The number of steps is a very determined value, no need to leave the runtime double comparison that task in my opinion.

@linev
Copy link
Member Author

linev commented Apr 25, 2025

@ferdymercury

There are several other potential problems:

  • dy can be 0 after gStyle->SetHatchesSpacing(0), negative values also will be problematic
  • vertical hatches not handled but emulated with angle 89.99 making artifact with fill style 3409 from core/base/doc/macros/fillpatterns.C macros
    fillpatterns

I do not think that few more double operations are real problem here.

@linev
Copy link
Member Author

linev commented Apr 25, 2025

void SetHatchesSpacing(Double_t h) {fHatchesSpacing = TMath::Max(0.1,h);}

There is protection.

Copy link

Test Results

    18 files      18 suites   4d 1h 33m 46s ⏱️
 2 731 tests  2 730 ✅ 0 💤 1 ❌
47 719 runs  47 718 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 0d2f8c4.

@linev linev merged commit 122f0bb into root-project:master Apr 25, 2025
19 of 22 checks passed
@linev linev deleted the pad_hatches branch April 25, 2025 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants