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

Add new methods to get the number of actions running in a given node with a specific tag. #16789

Merged
merged 3 commits into from Nov 4, 2016
Merged

Add new methods to get the number of actions running in a given node with a specific tag. #16789

merged 3 commits into from Nov 4, 2016

Conversation

@N2OMatt
Copy link
Contributor

@N2OMatt N2OMatt commented Nov 1, 2016

This is useful for cases that we want to know how many
animations with a specific tag is running in our target.
For example:

auto tag1Count = this->getNumberOfRunningActionsByTag(kMyTag1);
auto tag2Count = this->getNumberOfRunningActionsByTag(kMyTag2);

While this could be achieved (more or less) with callbacks
to increment the tagCount at start of action and decrement
at the end the proposed API is much more concise and less error
prone.

given node with a specific tag.

This is useful for cases that we want to know how many
animations with a **specific** tag is running in our target.
For example:

auto tag1Count = this->getNumberOfRunningActionsByTag(kMyTag1);
auto tag2Count = this->getNumberOfRunningActionsByTag(kMyTag2);

While this could be achieved (more or less) with callbacks
to increment the tagCount at start of action and decrement
at the end the proposed API is much more concise and less error
prone.
auto limit = element->actions->num;
for(int i = 0; i < limit; ++i)
{
auto action = (Action *)element->actions->arr[i];
Copy link
Contributor

@minggo minggo Nov 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is better to use static_cast<Action*> instead

Copy link
Contributor Author

@N2OMatt N2OMatt Nov 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@minggo I just used the same pattern of the "surrounding" methods.
I agree with the static_assert, but we should change for the other methods too?

Copy link
Contributor

@minggo minggo Nov 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are appreciated if you can help to fix other methods.

* @see getNumberOfRunningActionsInTarget
* @js NA
*/
size_t getNumberOfRunningActionsInTargetByTag(const Node *target, int tag);
Copy link
Contributor

@minggo minggo Nov 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is better to add const like

size_t getNumberOfRunningActionsInTargetByTag(const Node *target, int tag) const;

Copy link
Contributor

@minggo minggo Nov 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and it is better to use reference instead like

size_t getNumberOfRunningActionsInTargetByTag(const Node &target, int tag) const;

or we should check the invalidation of target.

Copy link
Contributor Author

@N2OMatt N2OMatt Nov 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@minggo I just used the same pattern of the "surrounding" methods.
Actually I didn't understand your comment "it is better to add const like ...", my PR
is already using ``const Node *`, isn't it?

About the reference, I have no ideas what would be better, we have a lot of comments with this issue:

// FIXME: Passing "const O *" instead of "const O&" because HASH_FIND_IT requires the address of a pointer
// and, it is not possible to get the address of a reference

But this could go deeper in the engine architecture than I could make a clear statement about.
I leave to you guys decide what is better, regarding this modification.

Anyway is just let me know about that I'll happily make another PR with the changes!

Copy link
Contributor

@minggo minggo Nov 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, let's keep the same pattern at this PR

@@ -398,6 +398,32 @@ ssize_t ActionManager::getNumberOfRunningActionsInTarget(const Node *target) con
return 0;
}

// FIXME: Passing "const O *" instead of "const O&" because HASH_FIND_IT requires the address of a pointer
// and, it is not possible to get the address of a reference
size_t ActionManager::getNumberOfRunningActionsInTargetByTag(const Node *target,
Copy link

@cmzx3444 cmzx3444 Nov 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method name can be shorter? like getRunningActionsNumber?

Copy link
Contributor Author

@N2OMatt N2OMatt Nov 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same feel, but I just followed the pattern in the code!
The other methods are "overloaded" by adding the "byTag" suffix, so I did the same.

For the other side, I think that is easier to make a mental model with this name since
we are used to the other methods that uses the "byTag" suffix.

Copy link
Contributor

@minggo minggo Nov 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow the same pattern looks good to me

N2OMatt added 2 commits Nov 3, 2016
As discussed in #16789 issue thread @minggo asked
to change the old style of cast to newer, more safer
static_casts.
Normalize all the static_cast<Action *> to static_cast<Action*>
some places we have spaces, other places haven't.

Fix the missing parenthesis.
@minggo minggo added this to the 3.14 milestone Nov 4, 2016
@minggo minggo merged commit 6be4ec8 into cocos2d:v3 Nov 4, 2016
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants