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

Fixing the "Multiple overlap between two objects" issue, closes #1247 #1267

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
5 participants
@danim1130
Copy link
Contributor

commented Aug 13, 2014

This pull request is supposed to fix the multiple overlap issue. Before going into the technical details, a quick summary:

  • What does it do:
    • When using the overlap() function, because of the way the FlxQuadTree works, two of the same object usually overlapped in more than one quadrant. Because of this, the callback function(s) were called multiple times, possibly causing bugs (player getting hit multiple times, etc.) This pull request is supposed to fix this.
  • What will it break:
    • I've wrote the code so that almost no one should be affected should this commit ever get into the master branch. The default value of SingleOverlap is false, so it won't change anything with older projects. The only thing that could possibly cause some issue is if someone also uses the built-in IDs of FlxBasics. If this is a problem, I could add another ID, specifically used for sorting to the FlxBasics.
  • What about performance:
    • During testing, I haven't really seen any difference. Naturally, singleOverlap uses extra memory, but besides that, it gives the same performance as the regular overlap in all normal cases (in extreme cases (>2000 object colliding on Release) the original overlap did better)

Technical Details:

If SingleOverlap is used, instead of watching for overlaps in execute(), it will start checking the overlaps when an object to the A_LIST is added. For this to work properly, we first have to load in the B_LIST. After this, we add the first object to the A_LIST of one of the quadrants. When this happens, it will check the overlap with every object from B_LIST, and while doing so, adds every checked object to the _checked SearchArray. When it finished the quadrant, and goes recursively deeper, it will check with the SearchArray every time an object is being checked for overlap. If an object has been found in the array, it will get ignored, if not, it adds to the array, then checks for the overlap. After the recursive functions have finished, we reset the SearchArray, and add the next object from the group to the A_LIST.
The method is the same if _useBothList == false.

The SearchArray uses a binary search algorithm. It uses the ID of the FlxObjects, which I made to be a unique number for every FlxObject.

danim1130 added some commits Aug 13, 2014

Updated FlxBasic to help with sorting.
Generating a unique ID for every FlxBasic to help with the sorting.
@MSGhero

This comment has been minimized.

Copy link
Member

commented Aug 13, 2014

Does it have to be this complicated? Wouldn't a Map<AList item, BList item> work all the same? id is used in flixel-ui so I'm not sure this works everywhere.

@danim1130

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2014

I didn't do that, since that way, the Map could grow really big really quick. It could essentially become ObjectOrGroup1.size * ObjectOrGroup2.size (ObjectOrGroup1.size * (ObjectOrGroup1.size - 1) if using only A_LIST), and searching in a map that big could affect the speed if any of the group is big enough, while this way, the array will become at max ObjectOrGroup2(or 1).size. By the way, I wasn't perfectly clear about that part. Since the ID assignment happens in the constructor, you can freely change it after the object has been constructed. The only problem that could happen in this case would be if you tried to overlap that object with another object of the same ID while using the FlxG.overlap() function with SingleOverlap. Seeing as you probably (altough I'm not sure, I've never worked with flixel-ui) don't overlap flixel-ui objects, it won't be a problem, but if you still want to overlap it, you can, just not with SingleOverlap.
Of course, if the community think that performance isn't as important as changing the ID, I will gladly rewrite it to use Maps.

@MSGhero

This comment has been minimized.

Copy link
Member

commented Aug 13, 2014

I mean that id or ID is already taken and might cause compiler errors when using other libs. Though if the Travis build worked it should be fine. It's just weird and seems unnecessary.

Map access is theoretically faster than searching through an array (O(log n) vs O(n)), and you're only adding successful collisions not the entire length of either list most of the time. Since this is performance-critical code, both ways should be profiled in every situation on several platforms to see which is more performant. And remember to clear the map after you're done for the gc.

@danim1130

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2014

The ID-s I'm using have already been added to the HaxeFlixel library, they just weren't used for anything as far as I know (even the API state this : IDs seem like they could be pretty useful, huh? * They're not actually used for anything yet though.).

As for the array, I'm not using normal arrays. If you check my implementation of SearchArray (added at the bottom of FlxQuadTree since at the moment, it's not suited for general use) it uses binary search, so the search time is O(log n) as well, while n is smaller than it would be when using one map throughout the whole overlap checking. I also don't only add every successful overlap, but every checked overlap (even if it doesn't actually overlap), so that the helping internal variables have to be only calculated once for every A_LIST object.

I still think my SearchArray implementation is better than using Maps, but you gave me an idea of only adding the successful overlaps to it. I will give it a try and see if it comes up faster than my original implementation.

EDIT : Yep, it became a little faster with larger group overlaps.

Updated FlxQuadTree to make it faster
This update should make the whole overlapcheck faster. While theoretically it makes the smaller group overlap tests slower, it's not by a noticeable amount, while making the larger group overlap (>2000) test faster, and uses less memory. The _checked SearchArray will now only store the objects that do overlap with the object from the A_LIST that's being added.
@MSGhero

This comment has been minimized.

Copy link
Member

commented Aug 13, 2014

Idk I just feel like this could be a 20 line solution by creating or reusing a map, adding collisions to it, checking if that collision already exists, and clearing the map. The ID thing seems like overkill but whatever. SearchArray is kinda random.

_notifyCallback = NotifyCallback;
_processingCallback = ProcessCallback;

if(_singleOverlap = SingleOverlap)

This comment has been minimized.

Copy link
@gamedevsam

gamedevsam Aug 13, 2014

Member

Typo, should be ==?

Ohh I see, this is misleading, I would prefer to refactor this:

if(SingleOverlap)
{
    _singleOverlap = SingleOverlap;
    ...

This comment has been minimized.

Copy link
@danim1130

danim1130 Aug 13, 2014

Author Contributor

Sure, I write my codes in my own style, and for some reason I like using that shortcut, but I admit that can be really misleading.

@gamedevsam

This comment has been minimized.

Copy link
Member

commented Aug 13, 2014

Using a Map would be a terrible idea in this case, Maps are slow data structures and should be avoided at all costs within functions that run every frame.

The proposed solution is pretty reasonable, but I fear it will have a very noticeable performance impact. I'd be interested to learn the performance difference on the collision demo with this optional setting enabled.

@danim1130

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2014

I've wanted to do the Map version for performance testing, but couldn't figure it out how to use it, since to the same object, more than one object will correspond. So if objectA1 overlaps with objectB1 and objectB2, I can't add both of them to a Map(Alist item, Blist item). The only way this could work is if I used Map(Alist item, FlxLinkedList) , and iterate through the LinkedList. For that, here's the source code :

http://pastebin.com/V518YBBt

It actually performed better than I expected, but the Map's size started to slow it down at bigger collisions (My newer solution could run the Collision demo with 1000*2 box at 30 fps, while the map got only 20.

@danim1130

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2014

I've made a little testing program based on the CollisionAndGrouping demo, and compiled it for flash and windows. Note, that these versions are Debug versions (Release versions can handle much more), but it should give a rough idea about the performance differences.

Flash : http://www.fastswf.com/EwlgtbY
Windows bin : https://drive.google.com/file/d/0B4btQYQ6keoka2JibndpZFVIWGc/

Overall, I would say that when the A_LIST is only a few objects, SingleOverlap will give around the same performance, while with more objects (for example, have the groups collide) the original collision will most likely outperform the SingleOverlap. Also, I haven't really been changing worldDivisions, but that also affects the performance. It is possible, that at higher worldDivisions (around 10-14) SingleOverlap is better, since the normal collision calls the separate function much more.

Also note that I've found another improvement, but it could possibly create a memory leak, (I'm not that advanced with haxe), see latest commit.

Update FlxQuadTree.hx
Using a signal to destroy _checked seems like the best of both worlds.
@danim1130

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2014

Should I add something else, or is this good enough for a merge?

@gamedevsam

This comment has been minimized.

Copy link
Member

commented Aug 18, 2014

I think it's good, but I'll let @Gama11 review everything one more time and do the merge.

@Gama11 Gama11 changed the title Fixing the "Multiple overlap between two objects" issue Fixing the "Multiple overlap between two objects" issue, closes #1247 Aug 20, 2014

private static var counter : Int = 0;

/**
* Used for fast sorting.

This comment has been minimized.

Copy link
@Gama11

Gama11 Aug 20, 2014

Member

We can't use ID for this, users might have their own for them. Nothing stops them from messing with the value, either.

@@ -323,9 +323,10 @@ class FlxG
* @param ObjectOrGroup2 The second object or group you want to check. If it is the same as the first, flixel knows to just do a comparison within that group.
* @param NotifyCallback A function with two FlxObject parameters - e.g. myOverlapFunction(Object1:FlxObject,Object2:FlxObject) - that is called if those two objects overlap.
* @param ProcessCallback A function with two FlxObject parameters - e.g. myOverlapFunction(Object1:FlxObject,Object2:FlxObject) - that is called if those two objects overlap. If a ProcessCallback is provided, then NotifyCallback will only be called if ProcessCallback returns true for those objects!
* @param SingleOverlap Wether an overlap should be only checked between two objects or not. (Uses extra memory)

This comment has been minimized.

Copy link
@Gama11

Gama11 Aug 20, 2014

Member

This description and parameter name are extremely confusing if you don't know about the issue.

@Gama11

This comment has been minimized.

Copy link
Member

commented Aug 20, 2014

I'm not really sure whether or not this is a good idea to be honest. It seems like an ugly hack that adds quite a bit of extra code and complexity to the collision logic, even if optional.

I'm also not sure if the underyling issue is really that big of a deal, it hasn't been brought up very often and doesn't seem particularly limiting.

I'd like a few more opinions.

@MSGhero

This comment has been minimized.

Copy link
Member

commented Aug 20, 2014

If Maps aren't a good idea and ID might already be in use, you could just do it the simple way and add a CollisionPair object to an array. When you find a collision, check if the pairing already exists inside the array, if not add it. You'd need hundreds or thousands of things all colliding at the exact same time for the performance hit to be significant, so it'd affect BunnyMark's performance and probably little else.

Nape uses CollisionArbiter which has more logic like the collision normal, but that might be overkill here.

danim1130 added some commits Aug 20, 2014

@danim1130

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2014

Would it help if I made the SearchArray it's own generic class? And yeah, I agree it's quite hacky, my main goal was performance. And if this issue doesn't really come up often, I'll understand if you just close this request.

@danim1130 danim1130 closed this Sep 17, 2014

@Vascofr

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2014

I'd just like to say that I think this issue is a big deal, since it can introduce bugs which may go unnoticed, and/or are very hard to find.

I've made quite a few games with flixel and only now I know about this bug, and now I know that some of my games probably have subtle bugs because of this (such as "acceleration platforms" that may give the player incorrect speed values because of overlapping more times that it was supposed to. I suspected at the time that the speed was sometimes a bit off, but couldn't see anything wrong with the code, so I thought it was a problem with Flash's timers or something).

At the very least, functions such as FlxG.overlap should have a comment that warns about this behavior.

@danim1130

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2014

I can agree that this issue can provide quite a headache when it comes up, but unfortunately, this is the price for the quick overlap testing, and I don't think this could be accomplished without using a slower algorithm. If it's really a problem for you, you can always set FlxG.worldDivisions to 1. This could slow down your game if you have a large number of objects tested, but otherwise should be fine, and this will get rid of the multiple detection bug.
I agree that this should be documented, would make it easier for everyone who doesn't know every little bug in flixel.

@Vascofr

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2014

Ok, if it's well documented I don't see that much of a problem, the biggest problem to me is that people will not know about this issue. I actually found it a bit by chance, but could of easily spent several days debugging thinking the problem was somewhere else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.