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

Wrap registries in Arc to support non-const registries and catch bugs. #372

Open
rj00a opened this issue Jun 17, 2023 · 0 comments
Open
Labels
enhancement New feature or request

Comments

@rj00a
Copy link
Member

rj00a commented Jun 17, 2023

Describe the problem related to your feature request.

Vanilla Minecraft stores all sorts of things in registries -- blocks, block entity types, entity types, cat variants, items, chat types, biomes and dimensions are just a few. Registries are useful for modders since they can be extended at runtime (for instance, this is the process for adding a new item with fabric). Dynamic registries like biomes and dimensions are even modifiable by vanilla servers via the registry codec.

  1. Currently, Valence only allows for the modification of the dynamic, networked registries. Static registries like blocks and items are fixed at compile time using generated enums and const variables. This poses a problem if we want to support modded environments, since the static registries need to be extendable at runtime by plugins.

    However, pervasive use of registries presents a serious usability problem; nearly every API that uses registries in some way would have to take the registry as an argument. For instance, every call to set_block would have to take an extra &BlockRegistry and the system would have to use Res<BlockRegistry>. Vanilla Minecraft gets around this by storing the registries in global variables, but global mutable state is highly discouraged in Rust for a multitude of reasons.

  2. It is possible to modify registries when you're not supposed to. Modifying registries can alter the protocol in subtle ways and cause packet caches to become invalidated as a result. There is no error message if this happens. Vanilla Minecraft solves this by marking registries as "frozen" and disallowing modifications after startup. However, this is more restrictive than I would like.

  3. In valence_anvil, the chunk worker thread wants access to the biome registry to load biomes, but it can't access it because the registry is owned by the ECS. (Not a huge problem because we can make a mapping from biome names to BiomeId ahead of time and send that over to the worker thread, but it indicates a larger problem).

What solution would you like?

The solution to the above problems is to make the registries internally refcounted and cheap to clone. The registries would still be Resources for easy access.

  1. We pass in the needed registries to the object at construction time and store them there. Then, methods can simply access the registries through self.
  2. We can detect this by checking if the registry's refcount is >1. We can choose to either panic or log a warning and clone-on-write.
  3. Registries can be sent to other threads because they are not owned by the ECS anymore.

What alternative(s) have you considered?

Additional context

@rj00a rj00a added the enhancement New feature or request label Jun 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant