jamiebrynes7 / spatialos-sdk-rs

Rust integration of the SpatialOS C API bindings
Apache License 2.0
21 stars 6 forks source link

Support enums & schema entity as keys in schema maps #160

Closed jamiebrynes7 closed 4 years ago

jamiebrynes7 commented 4 years ago

This PR adds support for both generated enums and entity keys in schema maps.

For enums, just derive the traits required in the generated code.

For schema entities:

This solves some of the problems described in #153

codecov[bot] commented 4 years ago

Codecov Report

Merging #160 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #160   +/-   ##
=======================================
  Coverage   13.83%   13.83%           
=======================================
  Files          39       39           
  Lines        3779     3779           
=======================================
  Hits          523      523           
  Misses       3256     3256

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6d8ce3d...1226475. Read the comment docs.

jamiebrynes7 commented 4 years ago

Yup that had crossed my mind as well, although like you say I don't think it will happen much in practice.

I was trying to think of a better way to implement Ord on the underlying SchemaComponentData. We could read out the serialized write buffer into a byte array and then order based on that, but this seems rather inefficient and I'm not entirely sure of the characteristics of the byte array returned by the Worker SDK.

randomPoison commented 4 years ago

The goal of implementing Ord based on the pointers was in order to be able to derive Ord for Entity, right? If we were able to generate an Ord impl for schema data types using the method we discussed we should be able to derive Ord for Entity without needing to use the pointer addresses.

jamiebrynes7 commented 4 years ago

We don't know how to interpret the schema objects as the correct concrete type since all we have is a component ID. So we can't use any implementations of Ord for a component or schema type.

All the information we have available to us is the pointer address, the list of unique fields on the schema object, and the raw byte buffer (requires allocation).

(Sorry if I misinterpreted the link as I'm on mobile and GitHub has a tendency to behave oddly on mobile)

randomPoison commented 4 years ago

Ah yeah, you're totally right! For some reason I was thinking that we stored the components as trait objects 🤔

Would it be reasonable to implement Ord in terms of just the ComponentId keys in the map? It would mean that two Entity objects with the same set of components would have the same sort order, but maybe that's not a bad thing? If we're basing the sort order on pointer addresses, we'd get a more definite ordering between entities with the same set of components, but that ordering would be effectively arbitrary. Also, this would only matter for the case of map<entity, T>, which can't possibly be a common use case, right?

jamiebrynes7 commented 4 years ago

Would it be reasonable to implement Ord in terms of just the ComponentId keys in the map? It would mean that two Entity objects with the same set of components would have the same sort order, but maybe that's not a bad thing?

I don't think this would be a bad idea necessarily.

Also, this would only matter for the case of map<entity, T>, which can't possibly be a common use case, right?

Yeah I don't think this is a particularly common usecase.

I'm happy to change the impl if you feel the above would be a better fit :smile:

randomPoison commented 4 years ago

Honestly I don't feel too strongly one way other. Implementing Ord based on pointers doesn't feel quite right, but I'm also not super sure the other options are way better at this point. I'm cool to approve this as-is if you'd rather keep the current approach 🙂

jamiebrynes7 commented 4 years ago

Yeah I don't feel strongly either way either :joy:

In the interest of keeping things moving, I think I'll merge this as is - and make a note to revisit this in the future :smile: