-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
A rework of Proximity Detector System to fix #34286 #34361
Open
de0rix
wants to merge
3
commits into
space-wizards:master
Choose a base branch
from
de0rix:fix-34301
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+64
−36
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
S: Untriaged
Status: Indicates an item has not been triaged and doesn't have appropriate labels.
size/S
Denotes a PR that changes 10-99 lines.
labels
Jan 10, 2025
Wrong PR link in the description I think you wanted #34286 |
Indeed, thanks! |
lzk228
added
P3: Standard
Priority: Default priority for repository items.
T: New Feature
Type: New feature or content, or extending existing content
D3: Low
Difficulty: Some codebase knowledge required.
A: Science
Area: Science department, not including Silicons.
S: Needs Review
Status: Requires additional reviews before being fully accepted
and removed
S: Untriaged
Status: Indicates an item has not been triaged and doesn't have appropriate labels.
labels
Jan 10, 2025
metalgearsloth
requested changes
Jan 10, 2025
Content.Shared/ProximityDetection/Systems/ProximityDetectionSystem.cs
Outdated
Show resolved
Hide resolved
Content.Shared/ProximityDetection/Systems/ProximityDetectionSystem.cs
Outdated
Show resolved
Hide resolved
Content.Shared/ProximityDetection/Systems/ProximityDetectionSystem.cs
Outdated
Show resolved
Hide resolved
metalgearsloth
added
S: Awaiting Changes
Status: Changes are required before another review can happen
and removed
S: Needs Review
Status: Requires additional reviews before being fully accepted
labels
Jan 10, 2025
github-actions
bot
added
size/M
Denotes a PR that changes 100-999 lines.
and removed
size/S
Denotes a PR that changes 10-99 lines.
labels
Jan 12, 2025
github-actions
bot
added
S: Needs Review
Status: Requires additional reviews before being fully accepted
and removed
S: Awaiting Changes
Status: Changes are required before another review can happen
labels
Jan 12, 2025
de0rix
changed the title
A rework of Proximity Detector System to fix #34301
A rework of Proximity Detector System to fix #34286
Jan 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
A: Science
Area: Science department, not including Silicons.
D3: Low
Difficulty: Some codebase knowledge required.
P3: Standard
Priority: Default priority for repository items.
S: Needs Review
Status: Requires additional reviews before being fully accepted
size/M
Denotes a PR that changes 100-999 lines.
T: New Feature
Type: New feature or content, or extending existing content
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
About the PR
In this PR, the ProximityDetectionSystem.cs has been reworked.
It now can match entities if they have at least a single component from the list in yaml (i.e. properly handle component filtering with RequireAll being false), as well as preserving previous functionality of matching entities with all components in the same list.
Some code was also changed to handle tags search in a better way: now it's possible to use tags only as a criteria, as well as dropping the need to mark TagComponent specifically in the list of components in a criteria (because other way, there would be no entities with field Comp containing TagComponent, that is checked for by previous code) and making tags in a criteria work as a whitelist instead of a blacklist.
Why / Balance
This PR fixes #34286 and other similar subsequent issues, where entities that use this system and component need to match entities that have a single component listed in the criteria.
Technical details
All code logic changes are in the system.
Some formatting was changed.
Media
footage1.mp4
Requirements
Breaking changes
Possible breaking change is in how the matching entities by criteria is handled now. I've tested all of the two locators currently in-game (anomaly and spectral locator), and adjusted their prototypes a little bit to fix the issue #34286 or to better match new changes, but obviously, developers from other forks of the game should adjust their prototypes accordingly if needed.
Previously, it seems like tags criteria worked like a blacklist, so now entities that should be matched should have those listed tags, instead of entities, that should not.
Default for RequireAll is now false, and because now it affects how entities with listed components matched, changes should be made to their prototypes and/or setting this property to be true.
Changelog
🆑