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

EntitySize -> EntityDimensions #715

Closed
Sollace opened this issue May 27, 2019 · 12 comments
Closed

EntitySize -> EntityDimensions #715

Sollace opened this issue May 27, 2019 · 12 comments

Comments

@Sollace
Copy link
Contributor

Sollace commented May 27, 2019

EntityDimensions and all the fields there-in are indirectly confirmed by Mojang through string constants in their code:

   public String toString() {
      return "EntityDimensions w=" + this.width + ", h=" + this.height + ", fixed=" + this.constant;
   }

EntitySize -> EntityDimensions
EntitySize.constant -> EntityDimensions.constant

In addition to that, I would also change these two methods to be more in line with what they seem to be doing:

EntitySize.resizeable(...) -> EntityDimensions.changing(...)
EntitySize.constant(...) -> EntityDimensions.fixed(...)

@ChloeDawn
Copy link
Contributor

It was decided after discussion that EntitySize better represents this data class. I believe constant is fine too, representing an immutable size. Imo fixed doesn't really imply that well.

@Pyrofab
Copy link
Contributor

Pyrofab commented May 27, 2019

I am personally not fond of having class names differ from the string representation, especially when the name is not particularly bad. It's just confusing to have blatant conflicting information.

@liach
Copy link
Contributor

liach commented May 27, 2019

this mojang is indeed a bad obe. we have dimensions for worlds already! but the rename from constant to fixed i support.

@liach
Copy link
Contributor

liach commented May 27, 2019

on a side note, i initially mapped this class before mojang overrode the toString method. we should add to the guideline about whether we are mapping to mojang names (which are sometimes incorrect or off) or correct names.

@Runemoro
Copy link
Contributor

Runemoro commented Jun 4, 2019

EntitySize is wrong. The word "size" means only one value, and using it to refer to the dimensions of an object is bad English.

This class represents the dimensions of an entity (width, height), not the size of the entity (big, small). In fact, I think we already have another getEntitySize method somewhere (in the slime entity?) that returns a single number.

@Prospector
Copy link
Contributor

This would be a good issue to get a PR going for

@liach
Copy link
Contributor

liach commented Jun 4, 2019

EntityDimensions should be good as long as we call it Dimensions instead of Dimension. See the second definition at https://www.dictionary.com/browse/dimension

@Runemoro
Copy link
Contributor

Runemoro commented Jun 4, 2019

Yes, Dimensions, since it's a pair of two dimensions (width and height).

@Prospector
Copy link
Contributor

EntityDimensions is literally what it says in the issue name and string...

@Sollace
Copy link
Contributor Author

Sollace commented Jun 4, 2019

If everyone is on the side of doing a PR I can get one going later today.

Abd yes, I mean to say Dimensions not Dimension.

@Prospector
Copy link
Contributor

Yup, go for it

@Sollace
Copy link
Contributor Author

Sollace commented Jun 8, 2019

PR: #745

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

No branches or pull requests

7 participants