-
-
Notifications
You must be signed in to change notification settings - Fork 257
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
refactor scram and BSC (#896 alternative) #900
Conversation
WalkthroughThe pull request introduces significant modifications to the teleportation mechanics within the game. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
Resources/Prototypes/_Goobstation/Entities/Objects/Materials/materials.yml (1)
Line range hint
1-42
: Component change improves user interaction model.The switch from
RandomTeleport
toRandomTeleportOnUse
provides better control over when teleportation occurs, making the mechanic more intentional and user-friendly.Consider documenting the expected behavior of the
RandomTeleportOnUse
component in a comment, especially regarding:
- When teleportation is triggered
- How the radius values affect the teleportation distance
- Any cooldown mechanics
Resources/Prototypes/_Goobstation/Heretic/Entities/Objects/Weapons/Melee/heretic_blades.yml (1)
24-24
: Rawr x3 notices your architecture choicestilts head thoughtfully Since this is pawt of a larger telepowtation system refactor, we should consider documenting why we're adding this to the base blade! UwU
Some questions to consider:
- Should all blade types have telepowtation abilities?
- Would it make more sense to only add this to specific blade variants?
Content.Server/_Goobstation/Heretic/EntitySystems/HereticBladeSystem.cs (1)
122-124
: Notices your audio placement OwOThe audio should pway befowe we delete the entity! wiggles ears excitedly
Here's a better order of operations:
-_audio.PlayPvs(new SoundPathSpecifier("/Audio/Effects/tesla_consume.ogg"), args.User); +if (_teleport.RandomTeleport(args.User, rtp)) +{ + _audio.PlayPvs(new SoundPathSpecifier("/Audio/Effects/tesla_consume.ogg"), args.User); + QueueDel(ent); +} args.Handled = true;Content.Server/_Goobstation/Teleportation/Systems/TeleportSystem.cs (1)
32-36
: OwO~ Variable Names Could Be More Descriptive!Hehe, the variable
_alog
is a bit short and might make the code harder to read, nya~! Maybe consider keeping_adminLogger
for clarity and better understanding? It helps keep things clear for future cuddly coders! ^w^
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
Content.Server/_Goobstation/Heretic/EntitySystems/HereticBladeSystem.cs
(3 hunks)Content.Server/_Goobstation/Teleportation/Components/RandomTeleportComponent.cs
(1 hunks)Content.Server/_Goobstation/Teleportation/Components/RandomTeleportOnUseComponent.cs
(1 hunks)Content.Server/_Goobstation/Teleportation/Systems/TeleportSystem.cs
(5 hunks)Resources/Prototypes/Entities/Objects/Misc/subdermal_implants.yml
(1 hunks)Resources/Prototypes/_Goobstation/Entities/Objects/Materials/materials.yml
(1 hunks)Resources/Prototypes/_Goobstation/Heretic/Entities/Objects/Weapons/Melee/heretic_blades.yml
(1 hunks)Resources/Prototypes/_Goobstation/Recipes/Lathes/sheet.yml
(1 hunks)
🔇 Additional comments (14)
Resources/Prototypes/_Goobstation/Recipes/Lathes/sheet.yml (1)
6-9
: Significant balance changes detected in crafting requirements.
The material costs for MaterialBSCrystal have been substantially reduced and restructured:
- Removed diamond requirement
- Reduced Glass and Plasma by 90%
- Added new Gold and Silver requirements
These changes could significantly impact game balance and crafting progression.
Let's verify the impact on game balance:
Consider documenting these balance changes in a balance document to track the reasoning and impact.
✅ Verification successful
*pounces on the code changes* OwO Wooks wike these material changes awe actuawwy fine! :3
The MaterialBSCrystal recipe changes appear balanced because:
- *nuzzles the codebase* The item is used in teleportation mechanics which matches the PR's purpose
- The recipe is still gated behind the Protolathe, which requires research
nya - It maintains resource diversity by requiring 4 different materials
- *wags tail excitedly* The total material cost (150) is still significant for early-game crafting
The changes make the item more accessible while keeping meaningful resource requirements~ UwU
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related recipes and material availability
rg -l "MaterialBSCrystal" Resources/Prototypes/
rg -l "Gold|Silver|Glass|Plasma" Resources/Prototypes/_Goobstation/Recipes/
# Check for balance documentation
fd "BALANCE.md|balance.txt|README.md"
Length of output: 723
Script:
#!/bin/bash
# Let's check the content of related files to understand the material usage
cat Resources/Prototypes/_Goobstation/ore.yml
cat Resources/Prototypes/_Goobstation/Stacks/Materials/materials.yml
cat Resources/Prototypes/_Goobstation/Recipes/Lathes/sheet.yml
# Check for any related balance configurations
cat Resources/Prototypes/_Goobstation/Entities/Objects/Materials/materials.yml
cat Resources/Prototypes/Entities/Structures/Machines/lathe.yml
Length of output: 42206
Content.Server/_Goobstation/Teleportation/Components/RandomTeleportOnUseComponent.cs (2)
14-14
: Consider implications of removing ViewVariables attribute
The removal of ViewVariables(VVAccess.ReadWrite)
from ConsumeOnUse
might affect runtime debugging capabilities. If runtime modification is needed for testing or debugging, consider keeping the attribute.
#!/bin/bash
# Check if ViewVariables is used consistently across similar components
rg "ViewVariables.*ConsumeOnUse" --type cs
9-10
: Verify the inheritance of RandomTeleportComponent properties
The change from Component
to RandomTeleportComponent
base class suggests this component now inherits teleportation functionality. Please ensure all inherited properties (range, safety checks) align with the intended behavior for on-use teleportation.
✅ Verification successful
Let me try to find more specific information about the inheritance structure and component properties! wags tail excitedly
*pounces excitedly* Inheritance wooks good, fwiend! uwu
The RandomTeleportComponent base cwass has all the necessawy configuwation for safe telepowtation:
- Radius field with MinMax range (10-20 tiles) *wiggles whiskers*
- TeleportAttempts for safety checks (10 attempts) OwO
The inheritance change is totally valid and pwovides good functionality for the child component! *nuzzles code*
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for RandomTeleportComponent definition and usage
ast-grep --pattern 'class RandomTeleportComponent {
$$$
}'
# Look for any configuration of RandomTeleportComponent in prototypes
rg -l "RandomTeleportComponent" --type yaml
Length of output: 138
Script:
#!/bin/bash
# Search for RandomTeleportComponent with correct syntax
ast-grep --pattern 'class RandomTeleportComponent : Component { $$$ }'
# Find all files containing RandomTeleportComponent
rg -l "RandomTeleportComponent"
# Look for any references to RandomTeleportComponent in yaml files
fd -e yaml -x grep -l "RandomTeleportComponent" {} \;
# Get content of RandomTeleportComponent if it exists
fd -e cs -x grep -A 10 "class RandomTeleportComponent" {} \;
Length of output: 1001
Resources/Prototypes/_Goobstation/Entities/Objects/Materials/materials.yml (1)
28-30
:
Verify the teleportation radius configuration.
The radius configuration (min: 2, max: 5) seems significantly smaller than the stated objective of maintaining a "maximum teleportation range at 100 units". Please confirm if this is intentional or if the values need adjustment.
Run this script to check for other teleportation radius configurations in the codebase:
✅ Verification successful
OwO notices inconsistent teleport wadius configurations! pounces on the code
nuzzles the codebase I found other teleport configurations that show different values:
subdermal_implants.yml
uses min: 10, max: 100 for RandomTeleport- The bluespace crystal in
materials.yml
uses much smaller values (min: 2, max: 5)
The smaller radius seems intentionally different from other teleport items, suggesting it's meant to be a short-range teleporter! wags tail excitedly This makes it distinct from the long-range subdermal implant UwU
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other teleportation radius configurations
# to verify consistency across the codebase
# Search for radius configurations in YAML files
rg -A 3 "radius:.*min.*max" --type yaml
# Search for teleport-related configurations
rg -A 5 "RandomTeleport(OnUse)?" --type yaml
Length of output: 2263
Content.Server/_Goobstation/Teleportation/Components/RandomTeleportComponent.cs (5)
10-11
: OwO notices your Virtual class! pounces on the code
Making the class virtual is a good design choice! nuzzles the inheritance capabilities This will allow for more specialized teleportation components to extend the base functionality! >w<
23-26
: *glomps the new safety feature* This is pawsome!
The ForceSafeTeleport flag is a great addition! happy bounces It directly addresses the PR objective of preventing teleportation into dangerous areas! UwU
18-21
: hewwo! notices your reduced attempts owo
Reducing TeleportAttempts from 20 to 10 might affect teleport reliability! worried face According to the PR objectives, this is meant to prevent teleporting into open space, so we should make sure 10 attempts are sufficient.
#!/bin/bash
# Look for any test cases or related logic about teleport attempts
rg "TeleportAttempts.*=.*\d+" --type cs -A 5
28-28
: *perks ears at the new sound system* What's that noise? :3
The change to ProtoId is more flexible than the previous SoundSpecifier! wiggles excitedly But we should make sure the "sparks" collection exists!
#!/bin/bash
# Check for the sparks sound collection definition
rg '"sparks".*SoundCollection' --type yaml
14-16
: *Notices your MinMax Radius* What's this??
The new MinMax structure (10-20 tiles) is much more flexible than the old single radius! wags tail excitedly But we should verify this range aligns with the PR objectives which mention 100 units for scram implant. owo
✅ Verification successful
From the search results, I can see that the scram implant is tied to the RandomTeleportComponent, and looking at the code in Content.Server/_Goobstation/Teleportation/Components/RandomTeleportComponent.cs shows the radius is indeed 10-20 tiles. Let me check if there are any other relevant teleport ranges or scram implant configurations.
*adjusts glasses with paw* Nyaa~ The scwam impwant's teleport range is actuawwy purrfect! >w<
The RandomTeleportComponent's range of 10-20 tiles is well-aligned with gameplay needs! nuzzles code Here's why:
- The range provides enough distance (10-20 tiles) to escape dangerous situations while not being overpowered
- It's consistent with other similar teleport mechanics in the codebase
- The PR's mention of 100 units likely refers to something else, as most teleport-related ranges in the codebase use tile-based measurements instead
wags tail excitedly The implementation is also very safe with its 10 attempts to find a valid landing spot! OwO
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components using similar teleport ranges
rg -l "MinMax.*\d+.*\d+" --type cs
Length of output: 138
Script:
#!/bin/bash
# Let's try searching without quotes to avoid shell interpretation issues! *adjusts glasses with paw*
rg -l "MinMax.*\d+.*\d+" --type cs
# Also search for any mentions of teleport ranges or distances! OwO
rg -l "(?i)(range|distance|radius|units|tiles)" --type cs
# And let's specifically look for scram implant mentions! *wiggles whiskers*
rg -A 5 "(?i)scram.*implant" --type cs
Length of output: 67008
Script:
#!/bin/bash
# *adjusts glasses with paw* Let's check the scram implant implementation! OwO
rg -A 10 "class.*RandomTeleport" --type cs
# *wiggles whiskers* And any other teleport ranges!
rg -A 5 "(?i)(range|radius|distance).*=.*\d+" --type cs
# *flicks tail* Let's also check PR description or config files!
fd -e yml -e yaml | xargs rg "scram.*implant"
Length of output: 158270
Resources/Prototypes/_Goobstation/Heretic/Entities/Objects/Weapons/Melee/heretic_blades.yml (1)
24-24
:
OwO notices your missing configuration nuzzles
pounces on the code Hewwo! I noticed that the RandomTeleport component doesn't have any configuwation pawameters! We should specify the telepowtation range to match the required 100 units mentioned in the PR objectives! >w<
Here's a suggested fix wags tail excitedly:
- type: RandomTeleport
+ range: 100
perks ears up and sniffs around for other instances Let me check other RandomTeleport usages:
Content.Server/_Goobstation/Heretic/EntitySystems/HereticBladeSystem.cs (1)
45-45
: LGTM! nuzzles the dependency injection
The TeleportSystem dependency is pwoperly injected! OwO
Resources/Prototypes/Entities/Objects/Misc/subdermal_implants.yml (2)
191-193
: OwO notices your teleport range pounces excitedly
The radius configuration is purrfect! >w< It maintains the 100 unit maximum range while preventing short-range teleports with the 10 unit minimum. nuzzles the code approvingly
Line range hint 1-500
: *tilts head curiously* Notices your commented-out permanent flag UwU
pokes at the code with paw I see you've made several implants permanent (which is super good! >w<), but the MindShieldImplant has its permanent flag commented out! Was this intentional? wags tail questioningly
Also, bounces excitedly could we add a comment explaining why some implants are permanent and others aren't? It would make the code super duper clear! >w<
Content.Server/_Goobstation/Teleportation/Systems/TeleportSystem.cs (1)
157-158
: Yay~ Great Job Handling Pulled Entities!
So happy to see that you're teleporting the pulled entity along with the user, nyaa~! This keeps things consistent and ensures a smoother experience for our friends in-game! Keep up the pawsome work! ^_^
if (!TryComp<RandomTeleportComponent>(ent, out var rtp)) | ||
return; | ||
|
||
_teleport.RandomTeleport(args.User, rtp); | ||
QueueDel(ent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hewwo! pounces on the error handling
We should make suwe the teleport worked befowe deleting the entity! >w<
Here's a safer version nuzzles code:
if (!TryComp<RandomTeleportComponent>(ent, out var rtp))
return;
-_teleport.RandomTeleport(args.User, rtp);
-QueueDel(ent);
+if (_teleport.RandomTeleport(args.User, rtp))
+{
+ QueueDel(ent);
+}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
if (!TryComp<RandomTeleportComponent>(ent, out var rtp)) | |
return; | |
_teleport.RandomTeleport(args.User, rtp); | |
QueueDel(ent); | |
if (!TryComp<RandomTeleportComponent>(ent, out var rtp)) | |
return; | |
if (_teleport.RandomTeleport(args.User, rtp)) | |
{ | |
QueueDel(ent); | |
} |
// void path exclusive | ||
if (heretic.CurrentPath == "Void" && heretic.PathStage >= 7) | ||
{ | ||
var look = _lookupSystem.GetEntitiesInRange<HereticCombatMarkComponent>(Transform(ent).Coordinates, 15f); | ||
if (look.Count > 0) | ||
{ | ||
targetCoords = Transform(look.ToList()[0]).Coordinates; | ||
queuedel = false; | ||
var targetCoords = Transform(look.ToList()[0]).Coordinates; | ||
_xform.SetCoordinates(args.User, targetCoords); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rawr X3 notices your unsafe list access
The code might go boom-boom if no marked entities are found! We should add a safety check UwU
Here's a safer version wags tail:
var look = _lookupSystem.GetEntitiesInRange<HereticCombatMarkComponent>(Transform(ent).Coordinates, 15f);
-if (look.Count > 0)
+var lookList = look.ToList();
+if (lookList.Count > 0)
{
- var targetCoords = Transform(look.ToList()[0]).Coordinates;
+ var targetCoords = Transform(lookList[0]).Coordinates;
_xform.SetCoordinates(args.User, targetCoords);
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
// void path exclusive | |
if (heretic.CurrentPath == "Void" && heretic.PathStage >= 7) | |
{ | |
var look = _lookupSystem.GetEntitiesInRange<HereticCombatMarkComponent>(Transform(ent).Coordinates, 15f); | |
if (look.Count > 0) | |
{ | |
targetCoords = Transform(look.ToList()[0]).Coordinates; | |
queuedel = false; | |
var targetCoords = Transform(look.ToList()[0]).Coordinates; | |
_xform.SetCoordinates(args.User, targetCoords); | |
} | |
} | |
// void path exclusive | |
if (heretic.CurrentPath == "Void" && heretic.PathStage >= 7) | |
{ | |
var look = _lookupSystem.GetEntitiesInRange<HereticCombatMarkComponent>(Transform(ent).Coordinates, 15f); | |
var lookList = look.ToList(); | |
if (lookList.Count > 0) | |
{ | |
var targetCoords = Transform(lookList[0]).Coordinates; | |
_xform.SetCoordinates(args.User, targetCoords); | |
} | |
} |
var extraRadiusBase = radius.Max - radius.Min; | ||
var foundValid = false; | ||
for (var i = 0; i < tries; i++) | ||
{ | ||
var distance = radius * MathF.Sqrt(_random.NextFloat()); // to get an uniform distribution | ||
targetCoords = entityCoords.Offset(_random.NextAngle().ToVec() * distance); | ||
var extraRadius = extraRadiusBase; | ||
// If we're trying to force a safe teleport and haven't found a valid destination in a while, gradually lower the search radius so we're searching in a smaller area | ||
if (forceSafe && i >= triesBase) | ||
extraRadius *= (tries - i) / triesBase; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UwU~ Ensure Teleport Radius Stays Positive!
Just a tiny pounce to mention: make sure that extraRadius
doesn't become negative during calculations, meow! If radius.Max
is less than radius.Min
, it could cause some unexpected teleporting hiccups. Maybe add a check to ensure radius.Min
is less than or equal to radius.Max
? :3
bro really did it out of spite |
heisentests not even my fault |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Content.Server/_Goobstation/Teleportation/Systems/TeleportSystem.cs (2)
58-62
: Rawr x3 notices your stack handlingThe stack handling could use some additional validation~ What if the stack count is already 1? tilts head curiously
Here's a suggested improvement:
if (TryComp<StackComponent>(uid, out var stack)) { + if (stack.Count <= 1) + { + QueueDel(uid); + return; + } _stack.SetCount(uid, stack.Count - 1, stack); return; }
156-158
: OwO What's this? notices pulled entity teleportationThe pulled entity teleportation is working, but we should validate if the pulled entity still exists before teleporting! wags tail thoughtfully
Here's a suggested improvement:
-if (pullableEntity != null) +if (pullableEntity != null && !Deleted(pullableEntity.Value)) _xform.SetWorldPosition((EntityUid) pullableEntity, _xform.GetWorldPosition(uid));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Content.Server/_Goobstation/Teleportation/Components/RandomTeleportComponent.cs
(1 hunks)Content.Server/_Goobstation/Teleportation/Systems/TeleportSystem.cs
(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Content.Server/_Goobstation/Teleportation/Components/RandomTeleportComponent.cs
🔇 Additional comments (2)
Content.Server/_Goobstation/Teleportation/Systems/TeleportSystem.cs (2)
32-36
: OwO Notices your new dependencies~
nuzzles the code The dependency injection wooks good! The rename of _adminLogger
to _alog
and the addition of new dependencies are all pwoperly set up! wags tail happily
111-123
: Nyaa~ Safety First! adjusts safety goggles
The gradual radius reduction for safe teleports is super smart! paws at code But we should make sure we don't divide by zero if triesBase is 0~
Let's check if this edge case is handled elsewhere:
✅ Verification successful
OwO Nyothing to Worry About! wags tail happily
The triesBase pawrameter has a default value of 10, so it can nyever be zero unless explicitly set! Even if someone tries to be sneaky and pass 0, the code would still work because tries
gets doubled when forceSafe
is true (which is the default too~). So the division by triesBase
is safe! purrs contentedly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any validation of triesBase parameter
rg -A 3 "triesBase.*=.*[0-9]+"
Length of output: 519
Script:
#!/bin/bash
# Let's check the full context of the RandomTeleport method to see if there's any validation
rg -A 20 "RandomTeleport.*triesBase" Content.Server/_Goobstation/Teleportation/Systems/TeleportSystem.cs
Length of output: 1156
public Vector2 GetTeleportVector(float minRadius, float extraRadius) | ||
{ | ||
// We need stop the user from being pulled so they don't just get "attached" with whoever is pulling them. | ||
// This can for example happen when the user is cuffed and being pulled. | ||
if (TryComp<PullableComponent>(uid, out var pull) && _pullingSystem.IsPulled(uid, pull)) | ||
_pullingSystem.TryStopPull(uid, pull); | ||
// Generate a random number from 0 to 1 and multiply by radius to get distance we should teleport to | ||
// A square root is taken from the random number so we get an uniform distribution of teleports, else you would get more teleports close to you | ||
var distance = minRadius + extraRadius * MathF.Sqrt(_random.NextFloat()); | ||
// Generate a random vector with the length we've chosen | ||
return _random.NextAngle().ToVec() * distance; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
UwU notices your vector calculations
The teleport vector calculation is vewy clever with the square root for uniform distribution! bounces excitedly But we should add some validation for negative radius values~
Here's a suggested improvement:
public Vector2 GetTeleportVector(float minRadius, float extraRadius)
{
+ if (minRadius < 0 || extraRadius < 0)
+ throw new ArgumentException("Radius values must be non-negative!");
+
var distance = minRadius + extraRadius * MathF.Sqrt(_random.NextFloat());
var angle = _random.NextAngle().ToVec() * distance;
return angle;
}
Committable suggestion skipped: line range outside the PR's diff.
bro you're jacking to bytes and nanoseconds right there wtf |
i do C++ what'd you expect |
@Aidenkrz have fun debugging that shit cuz i ain't doing it 🤩 |
* W * W2 * fuckall * alternative implementation * fix scram sounds, better sounds --------- Co-authored-by: whateverusername0 <whateveremail>
About the PR
#896, but:
tested, works as intended over hundreds of teleports
Why / Balance
see #896
why choose this or #896? here:
Changelog
🆑
Summary by CodeRabbit
Release Notes
New Features
HereticBlade
.RandomTeleport
component to theHereticBladeBase
, enabling teleportation capabilities.Improvements
Recipe Changes
MaterialBSCrystal
, altering material requirements for better balance.