Aborting queries

[LegoCylon]

I'd find it useful to be able to bail out of a query by returning false via the callback. That kind of behavior is useful if you just want to do validation - i.e. are there at least N shapes in this area. Once you've found your minimum, additional work is wasted.

I'm willing to put in the plumbing for it and submit a patch for others to benefit from, but I wanted to mention it here first in case anyone had stipulations on the changes. The main concern I have is that the existing callbacks return void, so it seems like this would either need to be controlled with a #define or an alternate set of functions that take a different callback type.

[slembcke]

So I've thought about this before actually. It really wouldn't take that many changes. As you say though, it does break the API which puts it on the not-quite-written list of things I'm saving for the next version. Segment queries at the spatial index level have a hook for exiting early that I didn't expose at the query API level. I'd like to expose that as well, but again it would break the existing API.

For the BB tree, the changes are pretty straightforward. Just need to jump out of the recursion.
https://github.com/slembcke/Chipmunk2D/ ... ree.c#L349

The spatial hash changes would only be mildly harder:
https://github.com/slembcke/Chipmunk2D/ ... ash.c#L376

As for what to do with it... I'm not sure. I think it could be done with #ifdefs without making a huge mess. Really just around the callback definitions and where they are called? If you wanted to implement it that way, I'd include it. The other option is that I should start a Chipmunk 7 branch quite soon. I already have a couple misc branches where I've started experimenting with changes I want to make, but don't have anywhere to merge them to yet.

[LegoCylon]

Not surprised to learn it's already on your radar.
Thanks for the code references and feedback. I'll try the #ifdef approach.

[LegoCylon]

If you're considering broader changes in the future, it might be worthwhile converting the callbacks to use a structure parameter that contains the provided information. That way you could easily add information to the structure in a way that wouldn't break existing queries if they weren't updated to read from it. Or in this case you could have used it to provide a short-circuit variable without having to change the return type of the callback.

[slembcke]

I'm not quite sure I follow. So the user would have to provide a pointer to a ChipmunkNearestPointQueryOptions struct or something? That sounds like it would be messy inside and out of the API.

I guess I don't personally mind waiting a couple years to make minor changes like this. I somewhat feel like it gives me time to really think them through instead of making a series of impulsive changes that are harder to take back later.

For instance, one reason why the callbacks don't require you to return a value is keep the API simpler. How many people are really going to want to do an early exit vs. the number that would be confused why the callbacks require a value to be returned? If 99% of the usages are just going to return a particular value, it might be a bad design choice. On the flip side, how many realistic use cases where it would be handy is it really needed and would provide a measurable performance improvement? I personally have never written a game where queries even showed up on the performance profile (except in Unity...) and I've never heard feedback from anybody saying they weren't fast enough. Those are things I try to keep in mind.

For an open source library, I feel like it's reasonable not to implement every feature *everybody* could ever want if it seriously hurts the performance or usability in the general case. Closed source stuff can't be so easily adapted to do what you want if it doesn't do it out of the box

[slembcke]

The other possibility that requires no modification is to use a non-local jump. setjmp() on most machines is practically free to do. The only catch is that it would skip unlocking the space and you'd need to poke at the private API to reset it. I've considered making an API for that as well, but explaining why it's needed would also almost certainly create more confusion than it would solve problems.

Blah. I dunno. I guess what I'm getting at is that it solves a very specific problem that I think very few people ever have. I'm always very wary about implementing features like that on the chance they make it harder to implement something more important later on. I've done that a number of times.

[LegoCylon]

Totally understood. It was just a suggestion in case you felt like there were going to be further changes there anyway. Sorry if it came off as more insistent.

[slembcke]

Hah. No no that's fine. I was just in a weird mood yesterday where I felt like I needed to make a long explanation of it.

[LegoCylon]

It looks like segment queries use an internal callback when querying the spatial index which return a float used to compare results when doing a "First" query.

I doubt many people are manually calling into the spatial index to do a segment query so we could either change the return type to the continue/abort status for consistency or add another parameter to the callback to indicate that status (and do the same for non-segment queries).

I'm leaning towards an extra parameter that's only visible if a #define is turned on. Any thoughts?

[slembcke]

Well I wouldn't change the implementation of the segment query in the spatial index. If you want to abort a segment query, returning 0.0 will be enough to ensure a more or less immediate exit (unless start the query inside of multiple objects). Basically you are telling it how much far along the ray to continue searching for intersections. Only shapes or nodes that actually contain the start point fulfill that, and that's pretty rare.