BugFix: MsgSpatial interaction radius correct when not a factor of en…

[Robadob]

todo

• Fix bug
• Seatbelts error if wrapped is used when radius is not a factor of environment bounds
  • Happy for someone to update my fmod() solution to something cleaner, floating point isn't great.
• Test that bug is fixed
• Test that wrapped is blocked under SEATBELTS=ON
• document wrapped limitation (Clarify wrapped spatial message iterator bounds:radius limitation. FLAMEGPU2-docs#176)

Closes #1157

[Robadob]

I did run full test suite of a Debug build, just closed it without thinking to copy results.

[ptheywood]

Tests all pass under linux.

Code looks sane, other than maybe the comment about the epsilon value selected.
Prolly fine to merge but might be worth thinking about the epsilon / testing with some edge-casey (but still sensible, allocations will fail for very high bin counts) comm radiuses / env widths to make sure the epsilon check still behaves.

If / when we have a fp64 spatial comm (#959) that would need a different epsilon due to increased precision.

Approving anyway, but might warrent a chagne.

[ptheywood]

Any particular reason/justification for this particular epsilon value?.

Though our current spatial comms is fp32 only anyway, so a magic value is fine, just why this one / is it sufficient for different values

E.g. if the comm rad is a small positive number, will this work? (though if its too small then large env widths will cause other problems anyway, so can prolly get away with a fixed arbitrary epsilon)

Tests only appear to cover a single radii/width combo.

[Robadob]

Any particular reason/justification for this particular epsilon value?

Nope, as mentioned in OP...

Happy for someone to update my fmod() solution to something cleaner, floating point isn't great.

[Robadob]

(I'd rather make wrapped work at a tiny cost to perf for non-wrapped to avoid this floating point crap)

[ptheywood]

I agree that hurting wrapped more and not hurting non-wrapped is the better choice.

Given the radii and width are known on the host, we could maybe do the check there once, and then set a value on the device somewhere marking if wrapped is possioble for that message list or not? for less perf loss to wrapped usage. An extra global memory read is probably cheaper than a couple of fmodf's?

Could do it entirley on the host if we made wrapped opt-in on the message list, but I assume that's not desirable?

Not sure there's a better option than fmodf, jsut the particular epsilon might need some thought / broader testing.

[Robadob]

An extra global memory read is probably cheaper than a couple of fmodf's?

It's inside SEATBELTS, so perf doesn't matter.

…

On Wed, 13 Dec 2023 at 12:53, Peter Heywood ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In include/flamegpu/runtime/messaging/MessageSpatial2D/MessageSpatial2DDevice.cuh <#1160 (comment)>: > @@ -500,6 +500,13 @@ class MessageSpatial2D::In { // Return iterator at min corner of env, this should be safe return WrapFilter(metadata, metadata->min[0], metadata->min[1]); } + if (fmodf(metadata->max[0] - metadata->min[0], metadata->radius) > 0.00001f || I agree that hurting wrapped more and not hurting non-wrapped is the better choice. Given the radii and width are known on the host, we could maybe do the check there once, and then set a value on the device somewhere marking if wrapped is possioble for that message list or not? for less perf loss to wrapped usage. An extra global memory read is probably cheaper than a couple of fmodf's? Could do it entirley on the host if we made wrapped opt-in on the message list, but I assume that's not desirable? Not sure there's a better option than fmodf, jsut the particular epsilon might need some thought / broader testing. — Reply to this email directly, view it on GitHub <#1160 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFVGCQ25R2JNNNFQGAM2GDYJGQNZAVCNFSM6AAAAABARVOMGCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONZZGU3TKNRTGI> . You are receiving this because you authored the thread.Message ID: ***@***.***>