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

Add Generic instances for Component, Elem, Expl[Get|Set|Destroy|Members] #102

Open
Fizzixnerd opened this issue Mar 13, 2023 · 10 comments
Open

Comments

@Fizzixnerd
Copy link

I am in the midst of trying to add Generic instances for the above classes.

Rationale

It is my understanding that in order to currently use the library, things such as the following must be written:

newtype Position = Position { getPosition :: V3 Float }
instance Component Position where type Storage Position = Map Position
-- And so on, for say @Velocity and a @PlayerControlled tag...

-- We then define this type alias, which we must ensure smaller than an 9-tuple by nesting eventually.
-- This is undesirable because it is 1. annoying and 2. prone to error upon adding new components to the PlayerVehicle.
type PlayerVehicle = (PlayerControlled, Position, Velocity)

-- Then we use this in a system, using the PlayerVehicle type alias to help ensure correctness after adding new components to PlayerVehicle.
-- This means we have to update everywhere PlayerVehicle is used, even if we don't use the new added component in the system/whatever.
mySystem = cmap (\((tag, pos, vel) :: PlayerVehicle) -> (tag, pos + vel, vel)) -- pretend this makes sense
-- I omitted the newtype dance for brevity.

I would prefer to write instead something like the following:

-- UNCHANGED
newtype Position = Position { getPosition :: V3 Float }
instance Component Position where type Storage Position = Map Position
-- And so on, for say @Velocity and a @PlayerControlled tag...

-- CHANGED
data PlayerVehicle = PlayerVehicle { pvTag :: PlayerControlled, pvPos :: Position, pvVel :: Velocity } deriving (Generic)
instance Component PlayerVehicle

-- CHANGED
mySystem = cmap (\(v :: PlayerVehicle) -> v { pvPos = pvPos v + pvVel v }
-- One can imagine making this nicer with lenses, but that is kind of outside the point of this ticket.

Notice that adding a new field to PlayerVehicle does not change the implementation of mySystem.

Performance

apecs appears to be heavily performance focused, and I do not wish to introduce runtime overhead when unnecessary.

The proposed changes have some obvious (and probably not so obvious) performance implications, but I believe these are entirely opt-in; benchmarks would need to be performed before I could commit to this statement unqualified.

However, assuming the above point, this change would allow us to design our systems using components that are plain Haskell datatypes, instead of a mess of tuples of length < 9. If we find through benchmarking that some systems need to be optimized, so be it, switch those systems to tuples! But if they don't, then I believe this to be much better than the status quo, in terms of code maintainability and readability.

Work

I am already in the middle of performing the work, and will likely open a PR in the future with the necessary changes (adding G[Component,Elem,Expl[Get,Set,Destroy,Members]] classes/families and instances, and the default signature to the original non-G version of those classes/families). I thought this would be useful outside of my specific case however, and therefore am offering to do the necessary work to include it in apecs proper.

@dpwiz
Copy link
Collaborator

dpwiz commented Mar 13, 2023

I would like to require deriving Component via (Composite Map PlayerVehicle) newtype wrapper or something like this.

I can reasonably want to have a Generic instance for my own purposes and retain the type being opaque.
Having type elements automatically promoted to components may interfere with other components of the same type. Fields can clash even within a single type:

data PlayerVehicle = PV { origin, destination :: Position, fuel :: Fuel }

cmap \(PV{origin, destination}, Position current) -> -- ... 3 (!) components have a type of `Position`

Other than this, I'm with you here, this would be a nice thing to have (:

@Fizzixnerd
Copy link
Author

Fizzixnerd commented Mar 17, 2023

I would like to require deriving Component via (Composite Map PlayerVehicle) newtype wrapper or something like this.

Forgive me, but I'm not sure how this really differs from my original design, which required an explicit instance declaration for Component, as seen in the second code example. You can choose to derive this instance via anything you'd like, but I think that's sort of orthogonal to the Generic instances, in my opinion. Unless I've misunderstood something, which is entirely possible!

The implementation I was going with would in fact probably not allow the implementation for PlayerVehicle you have in mind above. It is entirely a way of amalgamating many existing stored values into a single data structure for easy reuse and manipulation. Therefore, origin and destination would be set to the same value always, since they are the same Position type, and so would have the same underlying Component instance.

To reiterate: One would declare Components as one normally does now. The only change here is that instead of manipulating [nested] tuples, you could use data structures of existing Components and they would transparently populate themselves using the underlying Storage of the fields.

@dpwiz
Copy link
Collaborator

dpwiz commented Mar 17, 2023

Ah, I think I get it now. I thought the proposal is to give a default Component implementation. If the fields are using their own stores, not just plastering Map over everything, then it's okay.

To check, the cmap and co would work without instance Component PlayerVehicle declaration?

@Fizzixnerd
Copy link
Author

They would not. PlayerVehicle would not be a Component, and therefore could not be used with cmap.

@dpwiz
Copy link
Collaborator

dpwiz commented Mar 19, 2023

I'm still confused..

So, the instance line in the original message is not required?

instance Component PlayerVehicle

@Fizzixnerd
Copy link
Author

If you omit the instance declaration instance Component PlayerVehicle, then PlayerVehicle will have a derived instance of Generic but no instance for Component. This would mean cmap could not be used with it. If you want to use cmap with a PlayerVehicle acting as a amalgam of Position and Velocity and PlayerControlled, then you must provide the instance declaration.

@dpwiz
Copy link
Collaborator

dpwiz commented Mar 20, 2023

  1. If I want my type (with a Generic instance) to have a store for itself and use arbitrary types in its fields I would write a proper Component instance manually.
  2. If I just want to use it as a shell for a bunch of properties, I will have to write (or derive-any-class) an empty Component instance so the proposed changes could kick in.

Looks reasonable 👍 I hope the performance would be retained.

@Fizzixnerd
Copy link
Author

I hope to come back to this in the coming weeks; have had to deal with some "real-life" issues.

@AphonicChaos
Copy link

Keep in mind there's a PR for v1.0 that's (to my understanding) stalled because of performance: #72 (comment)

I mention it here for two reasons:

  1. The API changes drastically
  2. You might be able to reuse the benchmarks there to confirm your suspicions about the performance of your changes

@MagicRB
Copy link

MagicRB commented Apr 2, 2024

This is tangentially related, but I implemented a kind of "fallback component store", where anything can be shoved into using TypeRep. The use case there is for prototyping and possibly very seldom used components. It is slow as it involves a TypeRep lookup and casting and other random stuff. If there is interest I may either publish or try to upstream

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