Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
added support for negative indices to 'getChildAt', 'addChildAt', and…
… 'removeChildAt'
- Loading branch information
1 parent
9462e87
commit 5232a92
Showing
2 changed files
with
50 additions
and
9 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! After this change, a lot of mistakes, is not added to the parent of a child!
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm ... I don't understand how this can have a side effect; the changes should apply only if the given index is negative, which always resulted in an exception before.
Can you give me a code example of one such exception? I must be overlooking something.
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the problem! This is due to Feathers LayoutGroup
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please return back to the code, this change should be done in conjunction with Feathers.
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see! I didn't think of that side effect, you're right. Thanks for reporting that!
Thankfully, there's no need to remove that feature completely; I just needed to revert the implementation of "addChild()". See: 9fd6733
Please try it out and let me know if that helps!
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now everything is fine! :) Thank you for your efficiency!
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I pretty don't like this change, as it's just syntactic sugar, which degradates performance a little bit (but every bit is counted one day).
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd agree if this code was part of the render loop; but it's in child management, which is far less critical. Furthermore, we're talking about one "if" per method, no additional function call, so I doubt that this would show any impact even on hundreds of iterations. In fact, when I'm able to reverse my very latest commit (9fd6733), "addChild" whould even be faster then before, because the function "numChildren" is called only once, not twice.
That said: this change can still be reverted if it causes any issues or is unpopular for some other reasons. I just don't think that performance speaks against it.
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using timeline based animations and it is quite common that I call addChildAt multiple times per frame per object (as it's in classic Flash), so I still consider each "sugar" unnecessary - especially in cases where I don't see any real bonus.
But my version is quite different from your already, because I didn't wanted to add Sprite3D as I will never use it in current project.
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding reducing numChildren call, it's questionable. You would have to count also the condition, which is in "addChildAt" even in cases, where it's called directly.
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've got one more condition, but spare one method call (numChildren). And the method call is definitely much more expensive than the condition.
As for Sprite3D: did you test the performance impact? I took great care to make the effect on pure 2D code as minimal as possible. That said: yes, there sure is some code that needs to be executed that hadn't been previously.
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not tested, I don't have that much time.. and if you would check my version, I'm modifying your code quite drastically. This changes would not be accepted in official repo, but in my case I don't care. Like here: Oldes@a64bc57 ;-)
As I said, every bit is worth the effort in final.
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, that will definitely be faster. But for the official version, I did unfortunately have to add that test; you wouldn't believe how developers are able to bend the API, LOL. 😉
No worries, I know that you've got more important things to do than test different Starling iterations. Thanks for taking the time to give me feedback on this (and other) changes, though — it's appreciated! And the last word is not spoken about that "negative index" case. I might limit it to just "getChildAt".
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a problem with Feathers ScrollContainer!
Number of children may be different !!!! In this.viewPort Because I could add children through addRawChild();
Please return all back!
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alevys — what do you mean, is there a new problem?
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YES!
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess since this commit? eed8696
Is that right?
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not on this.
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please speak in full sentences, Alexander, otherwise this is going to take a while. ;-)
What exactly is your problem, and what can I do to help?
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow this code. It reflects the essence of the problem.
5232a92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could reproduce the problem, thanks! Josh is already working on an update of Feathers to restore compatibility.