Skip to content

Conversation

@cloud9c
Copy link

@cloud9c cloud9c commented Feb 11, 2021

No description provided.

@marcofugaro
Copy link
Member

Hello, could you motivate the reason behind this PR?

@cloud9c
Copy link
Author

cloud9c commented Feb 11, 2021

Changing computeAABB() to updateAABB() for the Body class has been on the todo for a while. Also, the current format is computeX() and updateX(), where computeX() usually has parameters and returns a value, while updateX() has no parameters and changes internal variables. computeAABB does not return anything or accept parameters, and instead updates the internal AABB for the body. Therefore, I think it makes more sense to change it to updateAABB().

@marcofugaro
Copy link
Member

True, it was as @todo in a jsdoc comment.

However I don't know about this one, three.js uses the word compute for these kind of methods.

What do you guys think @stockhuman @codynova?

@codynova
Copy link
Member

codynova commented Mar 4, 2021

I do think it makes sense to bring the method name inline with the other patterns throughout Cannon, but I also see value in bringing our method names more inline with Three. We could consider changing our other update method names to be closer to the pattern in Three, but that seems like it could cause a lot of churn, and we'd have to rethink our method naming scheme. Let's wait and see what Mike thinks. @marcofugaro I think you get the final decision.

@stockhuman
Copy link
Member

Hey folks. As when I first saw this issue, I still broadly agree with the change & can now echo Cody.
Re: other methods, as much as Three is a natural complement to Cannon, this lib isn't an ecosystem add-on but instead its own thing, so having internally-consistent names that aren't necessarily mapped to Three's sounds fine by me.

I'd go ahead an honour the old todo.

@marcofugaro
Copy link
Member

Right, didn't notice that in the Body the pattern was update, let's change it!

@marcofugaro marcofugaro merged commit af52757 into pmndrs:master Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants