craftworkgames / MonoGame.Extended

Extensions to make MonoGame more awesome
http://www.monogameextended.net/
Other
1.44k stars 325 forks source link

Add usable extension to Camera.cs and also improve documentation a little #845

Closed MikDal002 closed 4 months ago

MikDal002 commented 7 months ago

Hello :) It is my first contribution here. I am trying to share things I've found useful :).

Gandifil commented 7 months ago

Hi! Thanks for your attention!

It's good, but why you don't add method directly (without extension class)?

MikDal002 commented 7 months ago

Hi! Thanks for your attention!

It's good, but why you don't add method directly (without extension class)?

Although it is a matter of style (I can join them if you want to), I think that increases readability because introduced extension method doesn't add anything unique to the Camera class. I am used to keeping concrete classes as consist as possible to not bloat it with unnecessary functionality, which increases the cognitive complexity.

AristurtleDev commented 4 months ago

Hi @MikDal002, first thank you for the PR submission.

Looking at these changes, I'm not sure there's a valid use case to add this extension method. The Camera<T> class is a generic, which means any implementation of it is not always going to be a Vector2 for the T type. The provided implementation in MonoGame.Extended (the OrthographicCamera) does happen to use Vector2 for the generic type, however consumers may choose to use any valid struct such as Vector3, or Point.

By adding this extension method, we would also need to add additional methods to support each scenario, which means MonoGame.Extended is now anticipating the needs of the consumer which goes against the Not Future Proof principle. Additionally, this extension method adds very little utility since the consumer can just do orthCamera.ScreenToWorld(point.ToVector2()) and achieve the same functionality, even though they have to use the Point.ToVector2() call, which is all this is doing internally already

Instead consumers that derive from Camera<T> should implement the T ScreenToWorld(T) method within the class they create to fit that use-case/scenario.