Skip to content

Conversation

@tlf30
Copy link
Contributor

@tlf30 tlf30 commented Jun 8, 2020

This PR adds the ability to remove MorphTargets from Mesh.
It also adds a helper function to get a single MorphTarget.

~Trevor

@Ali-RS
Copy link
Member

Ali-RS commented Jun 10, 2020

Note that if a mesh has no MorphTargets, then the morphTargets field will be null, and thus calling each of those newly added methods will throw NPE.

Maybe we should better to check if it is null then:

in case of boolean removeMorphTarget(MorphTarget target) return false
in case of MorphTarget removeMorphTarget(int index) throw IndexOutOfBoundsException
in case of MorphTarget getMorphTarget(int index) throw IndexOutOfBoundsException

Any thoughts?

@stephengold
Copy link
Member

My thoughts: avoid throwing NPE in these methods. Getter and index-based remove method should throw IndexOutOfBoundsException. Instance-based remove method should return false.

@pspeed42
Copy link
Contributor

If you have to pass an index in and the index is out of bounds then you should throw an index out of bounds exception. What sgold says. :)

@tlf30
Copy link
Contributor Author

tlf30 commented Jun 10, 2020

I had not considered when there are no morphs, I will update the PR when I get home tonight. As it stands a index out of bounds will get thrown when the index it out of range when not null.

@tlf30
Copy link
Contributor Author

tlf30 commented Jun 11, 2020

I have made the requested changes.

@Ali-RS
Copy link
Member

Ali-RS commented Jun 11, 2020

Looks fine to me, thanks.

@Ali-RS
Copy link
Member

Ali-RS commented Jun 12, 2020

If no objection, I am going to merge this in the next 24 hours.

@Ali-RS Ali-RS merged commit e78f303 into jMonkeyEngine:master Jun 13, 2020
@stephengold stephengold added this to the v3.4.0 milestone Mar 13, 2021
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