[kr1xh]

Description

Fix integer division in TMath::IsInside() when Int_t coordinates are used.

Problem

The current implementation calculates the edge intersection as:

(yp-y[i])/(y[j]-y[i])*(x[j]-x[i])

fixes ##23450

[linev]

@kr1xh

Can you extend test a bit?
Not just single point - which by chance may work or not.
But many points inside such diamond shape.
Like loop:

for (int x = 100; x < 500; ++x)
   for(int y = 100; y < 500; ++y)
       TestPoint(x,y, GetResultFromOtherMethod(x,y));

[github-actions]

Test Results

    24 files      24 suites   4d 1h 33m 1s ⏱️
 3 889 tests  3 888 ✅ 0 💤 1 ❌
83 246 runs  83 245 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 1624710.

♻️ This comment has been updated with latest results.

[kr1xh]

@kr1xh

Can you extend test a bit? Not just single point - which by chance may work or not. But many points inside such diamond shape. Like loop:

for (int x = 100; x < 500; ++x)
   for(int y = 100; y < 500; ++y)
       TestPoint(x,y, GetResultFromOtherMethod(x,y));

Thanks. I’ve expanded the regression test to cover many points across the diamond rather than a single point.

One question: would you also like me to add a few targeted cases for negative coordinates, vertices/near edge points and coordinates around ±10000 given the expected integer coordinate range mentioned in the issue?

I can add those if you think they would be useful.

[kr1xh]

@kr1xh

Can you extend test a bit? Not just single point - which by chance may work or not. But many points inside such diamond shape. Like loop:

for (int x = 100; x < 500; ++x)
   for(int y = 100; y < 500; ++y)
       TestPoint(x,y, GetResultFromOtherMethod(x,y));

Implemented the expanded regression test as suggested. It now compares the Int_t and Double_t results across the diamond-shaped region (x = 0..600, y = 500..850), covering 210,951 points.

[agheata]

This clearly improves on the previous version based on integer division. The calculation is exact provided the intermediate differences and cross products fit in Long64_t. However, even Int_t coordinates can overflow the products:

constexpr Int_t a = 1600000000;
Int_t x[] = {-a, -a, a, a};
Int_t y[] = {-a, a, a, -a};
TMath::IsInside(1500000000, 0, 4, x, y); // expected true

The point is strictly inside the square, but the tested build returns false. One product is −9,920,000,000,000,000,000, outside the signed 64-bit range, causing undefined behavior.

IMHO this is mergeable with a code comment documenting the overflow limitation, since it correctly handles typical bounded coordinate ranges. Runtime overflow checks may add overhead; their cost compared with using double would need benchmarking.

[linev]

@agheata

With previous implementation we got 50% of failures with "normal" values from range -1000 ... +1000.
Probably some comments about limitations would be nice - but after the fix one will be able to use method in the graphics where coordinates are never exceed 10000.

[linev]

I let @hageboeck review and approve PR

[kr1xh]

can I help with something? @linev @agheata @hageboeck @ktf

[ktf]

For the record, I am not sure why I am in the loop.

[kr1xh]

For the record, I am not sure why I am in the loop.

my bad

[linev]

I reopen PR to restart CI

[linev]

I think, new implementation is much better than before.
So let merge it!