search

jonascarpay / apecs

a fast, extensible, type driven Haskell ECS framework for games
392 stars 43 forks source link

apecs-stm: newEntity_ leads to a compilation error, not newEntity #118

Closed glocq closed 1 year ago

glocq commented 1 year ago

When using apecs-stm's version of newEntity_, I get the following error:

No instance for (ASTM.Has World IO Apecs.EntityCounter)
        arising from a use of ‘ASTM.newEntity_’

(World has been created using apecs-stm's version of makeWorld) (ASTM is the prefix under which I include Apecs.STM.Prelude)

This might very well be me using the library wrong, however when I replace newEntity_ with its non-underscored version newEntity, my code compiles just fine.

Since both functions are supposed to do pretty much the same thing, and since one (newEntity_) is reimported from apecs while the other (newEntity) is redefined, I suspect newEntity_ should have been redefined in apecs-stm but did not.

dpwiz commented 1 year ago

I wonder why a different EntityCounter is needed for STM worlds.

The definitions in STM and IO worlds are exactly the same, but its global component is wrapped in ReadOnly for IO world, but not STM. The next-, new-, and newEntity_ are identical up to that special ReadOnly setter.

@jonascarpay What was the idea there? Perhaps we can just use core EntityCounter everywhere?

jonascarpay commented 1 year ago

What was the idea there?

Not sure! I thought maybe it was a vestige of the old type class structure, but I can't find it in the history

Perhaps we can just use core EntityCounter everywhere?

Yeah, sounds like a great idea

dpwiz commented 1 year ago

I just checked and STM makeWorld is using the same makeWorldNoEC from the core, just replacing the EC with a custom one. With EntityCounter sharing this one may also go away.

makeWorldAndComponents may remain as a shortcut for using STM Map for tiny/starter apps.

But I think both makeWorldAndComponents should be deprecated in favor of direct use of makeWorld + makeMapComponentsFor. This would avoid the need to rewrite world initialization when an app grows over "all components are maps" and starts using caches etc.

jonascarpay commented 1 year ago

@glocq apecs-stm 0.2 was released, this should now be fixed. If it's not, please reopen this issue.

dpwiz commented 6 days ago

@jonascarpay What was the idea there? Perhaps we can just use core EntityCounter everywhere?

I've just found out why... The culprit is the Global Store for that EntityCounter which wraps different backend (TVar vs IORef). Ditto for Unique.

That means newEntity now can only be used from IO and lost STM atomicity. Oops.

I now wonder if the proper fix is to use TVar backends for Global and Unique too. apecs-stm could still be a meaningful package, providing stm-containers stores.

jonascarpay commented 8 minutes ago

I'm a little hesitant about using TVar's over IOVar's performance reasons, although I suppose I should look at some benchmarks. If the difference is negligible, we could definitely do that. If not, how would you feel about going back to #119's original proposition of just having separate implementations?