Skip to content

Conversation

@rkraneis
Copy link
Contributor

@rkraneis rkraneis commented Jun 5, 2020

  • add getters and setters that should be there just for symmetry reasons
  • add missing javadoc (also on existing methods)

@stephengold
Copy link
Member

Looks good to me. Thank you for your contribution to JMonkeyEngine!

@MeFisto94
Copy link
Member

MeFisto94 commented Jun 5, 2020

Small note: A proper title (Adding getters/setters to AnimComposer) or something would've been more clear.
removeCurrentAction is what we needed for the SDK as well (jMonkeyEngine/sdk@b56a3fa)

@rkraneis rkraneis changed the title Improve anim composer Adding getters/setters to AnimComposer Jun 5, 2020
@Ali-RS
Copy link
Member

Ali-RS commented Jun 5, 2020

Can you please add documentation on those methods as well?

@rkraneis
Copy link
Contributor Author

rkraneis commented Jun 5, 2020

Can you also add Javadoc on those methods, please?

Yes, will do. What is the policy in regards to authorship in source files? I'd change the class comment
from

/**
 * Created by Nehon on 20/12/2017.
 */

to

/**
 * {@code AnimComposer} is a Spatial control that allows manipulation of
 * armament (skeletal) animation.
 * 
 * @author Nehon
 * @since 20/12/2017.
 */

@Ali-RS
Copy link
Member

Ali-RS commented Jun 5, 2020

I have no idea about it, but I guess it might be better to do it in a separate PR.

@stephengold
Copy link
Member

stephengold commented Jun 5, 2020

I'm not aware of any policy, only practice.

Adding an "author" tag is fine. JMonkeyEngine usually does not use "since".

Personally, I don't like seeing "code" tags in javadoc. The context usually makes them redundant.

@MeFisto94
Copy link
Member

Personally, I don't like seeing "@code" in javadoc. The context usually makes it redundant.
If something that'd be @link or @see though and would make more sense like that.

@rkraneis
Copy link
Contributor Author

rkraneis commented Jun 5, 2020

In regards to documentation: I tried to keep it close the the already existing one. If there's differing guide lines, I can of course overhaul the whole class ;-)

@Ali-RS
Copy link
Member

Ali-RS commented Jun 5, 2020

Looks fine to me, thanks

@Ali-RS
Copy link
Member

Ali-RS commented Jun 6, 2020

If there is no objection, I am going to merge this PR in next 24 hours.

@stephengold
Copy link
Member

OK. Just remember to use "Squash and merge" instead of "Merge commit" please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants