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

Sort optimisation #946

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

JohnBlackburne
Copy link

This adds two functions and associated constants, implementing a faster sort at the cost of some flexibility. Rather than a function it sorts on one of x, y or z. Sorting on y is commonly used to draw an overhead view, so objects recede into the distance as they move up towards the horizon. z is unused in Starling so can be used for arbitrary data. x is for completeness. In a simple test it seems to provide a 10 - 20% speedup over sortChildren using a function.


A sort function that is more like Array.sortOn. Instead of using
function it sorts on a field, either x, y or z, specified by a
constant. x and y allow sorting on the two display axes, while z is
unused by Starling, so can be used to sort on arbitrary data.

A sort function that is more like Array.sortOn. Instead of using
function it sorts on a field, either x, y or z, specified by a
constant. x and y allow sorting on the two display axes, while z is
unused by Starling, so can be used to sort on arbitrary data.
Initial version had 'z' alongside 'x' and 'y', for sorting on arbitrary data. But DisplayObject has no 'z' field, and adding one made no sense as it‘s not for z-sorting. Instead add 'sort' field, update sort code and also make that the default.
@PrimaryFeather
Copy link
Contributor

Thanks a lot for the pull request, John! I appreciate your efforts! 😄

Have you made any performance tests comparing this implementation with the existing way of passing a function? How much faster is this in typical use cases? (Plus: AOT vs. JIT?)

Furthermore, did you also compare this implementation (using the integer properties) to what's happening if you pass a String ("x") to the method instead, and then sorting by that?

I'm a little hesitant to duplicate the existing sortChildren, so I'd like to know if it's worth it. 😉

@JohnBlackburne
Copy link
Author

JohnBlackburne commented Mar 7, 2017

I used the following code to test it, based on code in this thread, more a quick hack than a thorough test, like the code. The speedup with this is around 20%.

package
{
	import flash.display.Sprite;
	import starling.core.Starling;

	[SWF(width="640", height="480", frameRate="60", backgroundColor="#bfbfbf")]
	public class SortTest extends Sprite {
		private var _starling:Starling;

		public function SortTest() {
			_starling = new Starling(MainStarling, stage);
			_starling.start();
		}
	}
}

	import starling.display.*
	import starling.text.*
	import starling.events.*
	import flash.utils.*
	import starling.utils.*
 
	class MainStarling extends Sprite
	{
 
		private var text:TextField;
 
		private var arr:Vector.<DisplayObject> = new Vector.<DisplayObject>();
		private var arrNum:Vector.<Number> = new Vector.<Number>();
 
		private var startTime:Number;
		private var endTime:Number;
		private var container:Sprite;
 
		public function MainStarling()
		{
			addEventListener(Event.ADDED_TO_STAGE, Start);
		}
		private function Start():void {
 
				container = new Sprite();
				
				var tf:TextFormat = new TextFormat("Verdana", 16, 0, "left", "bottom");
 
				//OUTPUT TEXT
				text = new TextField(500, 500, "hello", tf);
				text.border=true;
				text.autoSize="vertical";
				addChild(text);
				text.alignPivot();
				text.x = stage.stageWidth/ 2;
				text.y = stage.stageHeight / 3;
 
				addSprites(30000,true);
				addEventListener(Event.ENTER_FRAME, Tick);
		}
 
		//ENTER FRAME GAME LOOP
		private function Tick(e: Event):void {
			//SORT ON
 
			text.text = "TIMED SORTING TEST";
 
			//STARLING_SORT CHILDREN
			randomize();
			startTime = getTimer();
			container.sortChildren(sortOnY);
			endTime = getTimer();
			text.text +="\n"+ "old sort: " + (endTime-startTime);
 
			randomize();
			//NEW STARLING_SORT CHILDREN
			startTime = getTimer();
			container.sortChildrenOnKey(DisplayObjectContainer.ON_Y);
			endTime = getTimer();
			text.text += "\n" + "new sort: " + (endTime-startTime);
		}
 
		private function sortOnY(object1:DisplayObject, object2:DisplayObject):int {
			return (object1.y - object2.y);
		}
 
		private function addSprites(num:int,convertToInt:Boolean):void {
 
			for (var i:int = 0; i < num;++i ){
 
				var m:Sprite = new Sprite();
				m.y = (Math.random() * 1000) - 500;;
				container.addChild(m);
 
				//convert .y to int so it can be sorted
				if (convertToInt){
					m.y = int(m.y);
				}
			}
		}
 
		private function randomize():void{
 
			for (var i:int = 0; i < container.numChildren;++i ){
				var sp:DisplayObject = container.getChildAt(i);
 				sp.y = (Math.random() * 1000) - 500;
			}
		}
	}

So probably not something that makes sense as a feature of Starling. Anyone who wants it can implement it themselves. You also don’t need to make such drastic modifications to Starling to do this: you can just make _children public or create and accessor to it then sort that.

Simplify. Now adds no public methods to class, and implements only one method, sorting on the 'sort' field, which it calls if no function is supplied.
@JohnBlackburne
Copy link
Author

I’ve uploaded another version, which simplifies how it works. Now instead of a new public method it instead works through sortChildren; if no function is supplied it calls the version without the function calls, which only has one way of working.

It’s basically the version I’m using; it was an idea I had during the discussions on the forum, and the easiest way to demonstrate it was code it, try it out and upload it. Apart from the above test code I wanted to test it in production code, to be sure it worked, even though the number of things I’m sorting is too small for sort performance to be an issue.

@kheftel-old
Copy link

I'm only one voice, but I just wanted to say that I think being able to sort by y at the system level would be a great feature, and a common enough usecase for a 2D framework that a case could be made for including it in core.

@JohnBlackburne
Copy link
Author

It does not actually sort on y, any more: I simplified it and left in only the sort on 'sort', a new field added to DisplayObject, as that was more useful to me. The cost of this sort of optimisation is less flexibility, and how much you leave in is pretty arbitrary. Right now if you want sort on y you can grab the code and change '.sort' to '.y' in mergeSortNoFunction.

@PrimaryFeather
Copy link
Contributor

Thanks for the new version, and thanks for your feedback, too, Kawika!

You're probably right, the performance enhancement alone is probably not big enough to justify adding the additional method. On the other hand, as Kawika said, a simple way to sort by "y" (or any other property) would simplify things. I'll definitely think about it!!

@swapnilkhairnar
Copy link

The main reason why its not working is in the roots of itself....i think neither y has any problem nor x problem is in tht arbitrary axis i.e the centre .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants