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

Expose SystemMeta field accessors (#5497) #7119

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

WinstonHartnett
Copy link

Adds user-facing immutable and unsafe mutable accessors to SystemMeta (see #5497).

Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the safety comments should give an indication on how to use them safely

@mockersf mockersf added the A-ECS Entities, components, systems, and events label Jan 7, 2023
@alice-i-cecile alice-i-cecile added the C-Usability A targeted quality-of-life change that makes Bevy easier to use label Jan 8, 2023
self.last_change_tick
}

/// Set this system's last change tick.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs links and warnings.

@@ -56,6 +86,18 @@ impl SystemMeta {
pub fn set_non_send(&mut self) {
self.is_send = false;
}

/// Returns this system's last change tick.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs links.

@@ -43,6 +43,36 @@ impl SystemMeta {
&self.name
}

/// Returns the system's component access set.
#[inline]
pub fn component_access_set(&self) -> &FilteredAccessSet<ComponentId> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should all be const, along with the other methods on this type.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wildly unsafe, but I agree that this should exist for advanced users to actually impl SystemParam manually. Needs more warnings, links and more helpful safety comments though.

@alice-i-cecile alice-i-cecile added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Jan 14, 2023
@james7132 james7132 self-requested a review January 15, 2023 19:03
@@ -43,6 +43,36 @@ impl SystemMeta {
&self.name
}

/// Returns the system's component access set.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Returns the system's component access set.
/// Fetches the system's component access set.

&self.component_access_set
}

/// Returns a mutable reference to the system's component access set.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Returns a mutable reference to the system's component access set.
/// Fetches a mutable reference to the system's component access set.

/// Returns a mutable reference to the system's component access set.
///
/// # Safety
/// This allows unsafe modifications to the component access set that can violate Bevy's scheduling soundness.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't documenting the actual safety invariant. Editing the component access set needs to accurately reflect the actual mutable and immutable accesses the system (or parameter) fetches from the World.

/// Returns a mutable reference to this system's archetype component access set.
///
/// # Safety
/// This allows unsafe modifications to the archetype component access set that can violate Bevy's scheduling soundness.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also must exactly reflect the safety invariants of using this.

/// # Safety
/// This allows unsafe modifications to the component access set that can violate Bevy's scheduling soundness.
#[inline]
pub unsafe fn component_access_set_mut(&mut self) -> &mut FilteredAccessSet<ComponentId> {
Copy link
Member

@james7132 james7132 Jan 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pub(crate) on the fields should be removed and the bevy_ecs uses of the access sets should use these accessors. Won't block on this, can be done in a followup PR.

@@ -15,13 +15,13 @@ use super::{In, IntoSystem, ReadOnlySystem};
/// The metadata of a [`System`].
#[derive(Clone)]
pub struct SystemMeta {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this gets rebased, please also make the spans public too.

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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Complex Quite challenging from either a design or technical perspective. Ask for help!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants