Skip to content

Consider unsealing the Point class #30

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

Open
dustinandrews opened this issue Oct 18, 2019 · 4 comments
Open

Consider unsealing the Point class #30

dustinandrews opened this issue Oct 18, 2019 · 4 comments

Comments

@dustinandrews
Copy link

dustinandrews commented Oct 18, 2019

Edit: I guess I should say "convert from struct to class". My editor said it was "sealed" but I checked the code and it's a struct. Maybe you have compelling perf reasons to keep it a struct.

I was very surprised that I couldn't subclass the Point class. I know I can use composition instead. I'm using an ECS pattern where components implement an IComponent interface (for serialization). Some of the components are just points with a different name. LocationComponent and DestinationComponent for example. It would be a lot cleaner for them to inherit point so I can do things like subtract them.

var delta = location - destination
entity.AddComponent(delta);

V.S.

var delta = location.point - destination.point
var deltaComponent = new DeltaComponent(){point = delta};
entity.AddComponent(deltaComponent)

@FaronBracy
Copy link
Owner

Thank you for the feedback. It was originally a class but got converted to a struct in this commit:

87b1568#diff-336d9b5ef45c6e307febd568f0845ed0

People generally prefer it a struct for "performance" reasons, but in any of my performance profiling I haven't seen a big difference either way. I feel like it's a situation where it won't ever please everyone. I'll need to think about and research it some more before changing it back.

In the meantime could a potential workaround be to override the - operator on your component classes that are composed of points?

@dustinandrews
Copy link
Author

If it's not a problem in the profiler, it's not a problem! Anyone who says differently is guilty of premature optimization IMO. In the mean time I wrote a wrapper class so I can make things points and hand them to RogueSharp and back easily. It's not so much that I needed the - operator as I wanted to be able to interchange the points easily.

@henrikweimenhog
Copy link

I would much rather prefer an interface which has X and Y. This can then be used by Point, Actor, Cell, Behaviour, Item, etc, etc.
It would also be the input to all these methods that takes int x, int y. It greatly improves the extensibility and also gets rid of a lot of code.

@ekolis
Copy link

ekolis commented Mar 6, 2021

I suppose a record or a ValueTuple could be used, too?

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

4 participants