Skip to content

Split Resource and Component Access #14472

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

Closed
wants to merge 3 commits into from

Conversation

re0312
Copy link
Contributor

@re0312 re0312 commented Jul 25, 2024

Objective

Solution

  • Added a resource access in SystemMeta

@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Jul 25, 2024
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels Jul 25, 2024
@re0312 re0312 marked this pull request as draft July 25, 2024 11:39
@re0312 re0312 marked this pull request as ready for review July 25, 2024 13:41
@hymm hymm added the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Jul 25, 2024
@hymm
Copy link
Contributor

hymm commented Jul 25, 2024

Note that this method will double the memory used for access, since resources and components share the same id space. But that would get fixed if #14385 gets merged.

@cBournhonesque
Copy link
Contributor

Very nice!
Do you think an alternative approach of introducing read_all_components/write_all_components on top of read_all/write_all would work?

@re0312
Copy link
Contributor Author

re0312 commented Jul 28, 2024

Very nice! Do you think an alternative approach of introducing read_all_components/write_all_components on top of read_all/write_all would work?

it appears to be a better solution than this pr.

@alice-i-cecile
Copy link
Member

Do you think an alternative approach of introducing read_all_components/write_all_components on top of read_all/write_all would work?

This is my preferred approach here I think.

@cBournhonesque
Copy link
Contributor

Very nice! Do you think an alternative approach of introducing read_all_components/write_all_components on top of read_all/write_all would work?

I'm not sure what I suggested is possible actually. Because then you would also need to be able to track if an access has any_read_component or any_write_component

@cBournhonesque
Copy link
Contributor

Very nice! Do you think an alternative approach of introducing read_all_components/write_all_components on top of read_all/write_all would work?

I'm not sure what I suggested is possible actually. Because then you would also need to be able to track if an access has any_read_component or any_write_component

Actually I have a working version here: #14561

@alice-i-cecile
Copy link
Member

Closing in favor of #14561.

github-merge-queue bot pushed a commit that referenced this pull request Aug 6, 2024
# Objective

- Fixes #13139
- Fixes #7255
- Separates component from resource access so that we can correctly
handles edge cases like the issue above
- Inspired from #14472

## Solution

- Update access to have `component` fields and `resource` fields

## Testing

- Added some unit tests
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-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect handling of EntityRef/Mut Queries + Resources resulting in false conflicts
4 participants