Skip to content

Conversation

@alice-i-cecile
Copy link
Member

Objective

Solution

  • rename field
  • improve doc comments

Additional Context

In an ideal world, this would look something like

enum AccessLevel {
   Nothing,
   Reads,
   Writes,
}

Unfortunately, this is perf-critical code, so we're using a bitset to allow for quick comparisons.

If and when we want to extend Access to support more exotic access types (like presence for the existence of a component type or forge for the ability to add components of that type), the name makes even less sense.

@alice-i-cecile alice-i-cecile added the A-ECS Entities, components, systems, and events label Mar 23, 2022
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 23, 2022
@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Mar 23, 2022
@bjorn3
Copy link
Contributor

bjorn3 commented Mar 23, 2022

this field is confusingly named, as the ability to write to a field necessarily implies the ability to read

Not necessarily. You could make a SystemParam which can only write and not read.

@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Mar 23, 2022

You could make a SystemParam which can only write and not read.

This isn't really representable under Rust's aliased mutability rules, and it would not be any easier to schedule. I would actually like to add a debug-assert to verify the subset invariant, but I decided to keep it out of this particular PR.

@cart
Copy link
Member

cart commented May 3, 2022

The idea here was to be explicit that this isn't "exclusively reads" (which would be an easy assumption to make for something called reads when writes exists).

reads_and_writes could definitely be misinterpreted as "exclusively reads and writes", but that feels less risky here in the global context of the type. Happy to consider renames (especially ones that are completely unambiguous), but I personally don't think that as written this is a positive step towards more safety.

@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented May 3, 2022

Going to close this out. There's a clearer model to be used here (the 4 access level model described in the old Forge component RFC), and I think we should just migrate to that.

See also #4624.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Docs An addition or correction to our documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants