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.

Sign up for GitHub

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

Jump to bottom

Fix laser beam visuals in space #28797

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Fix laser beam visuals in space #28797

wants to merge 10 commits into from
+34 −43

Conversation

n00kii
Copy link

@n00kii n00kii commented Jun 9, 2024 •
edited
Loading

About the PR

fixed the visual effects for hitscan weaponry when fired between grids or in space (resolves #23699)

Why / Balance

bugfix

Technical details

add an extra field to HitscanEvent to keep track of the grid that the shot was fired from, attach all effect segments to that grid. because post-spawn grid traversal, requires an engine change to allow traversal to be bypassed and keep the effects attached to the source grid.

Requires space-wizards/RobustToolbox#5365

Media

previous behavior

laser-bugged.mp4

new behavior

laser-fixed.mp4

[X] I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Breaking changes

Changelog

🆑

  • fix: Fixed misalignment of laser effects when shot across grids.

@Plykiya
Copy link
Contributor

Plykiya commented Jun 9, 2024 •
edited
Loading

oh cool, I guess I can add HYPER CHARGED LIGHTNING back to the Tesla then

@Plykiya Plykiya mentioned this pull request Jun 9, 2024
Closed
5 tasks
@Partmedia
Copy link
Contributor

Partmedia commented Jun 13, 2024 •
edited
Loading

Thanks for the investigation and fix. In the future, please do not put "code cleanup" like variable renames in the same diff as this makes it harder and take longer to review. Would you mind reversing the variable renames? Otherwise, I can do that for you.

@n00kii
Copy link
Author

n00kii commented Jun 13, 2024

oh yeah sure thing

metalgearsloth
metalgearsloth requested changes Jun 14, 2024
View reviewed changes
Copy link
Contributor

@metalgearsloth metalgearsloth left a comment

Choose a reason for hiding this comment

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

Won't this break if the shuttle is moving at all if we're relying on mapcoordinates when networked.

@metalgearsloth metalgearsloth added the S: Awaiting Changes Status: Changes are required before another review can happen label Jun 14, 2024
@github-actions github-actions bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Jun 15, 2024
@github-actions GitHub Actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Jun 17, 2024
@n00kii
Copy link
Author

n00kii commented Jun 17, 2024

yeah so im not sure who made the code for replicating visuals but it could use some love. anyways fixed it while retaining netcoords, ready for review again

@n00kii n00kii requested a review from metalgearsloth June 17, 2024 08:26
@github-actions 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 Jun 17, 2024
@metalgearsloth 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 Jul 17, 2024
@n00kii
Copy link
Author

n00kii commented Aug 17, 2024

i think the checks have to be rerun since i put in the silly "requires" line in too late

@slarticodefast
Copy link
Member

i think the checks have to be rerun since i put in the silly "requires" line in too late

I tried rerunning them, but it seems like it did not register the engine requirement. Could you push an empty commit?

@n00kii n00kii requested a review from metalgearsloth August 19, 2024 22:57
@github-actions 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 Aug 19, 2024
@SlamBamActionman SlamBamActionman added T: Bugfix Type: Bugs and/or bugfixes S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. and removed PR: Bug Fix labels Nov 14, 2024
@SaphireLattice SaphireLattice added P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. D1: High Difficulty: Extensive codebase knowledge required. A: Combat Area: Combat features and changes, balancing, feel and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Nov 15, 2024
@TheShuEd TheShuEd assigned TheShuEd and unassigned TheShuEd Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@metalgearsloth metalgearsloth Awaiting requested review from metalgearsloth metalgearsloth is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
A: Combat Area: Combat features and changes, balancing, feel D1: High Difficulty: Extensive codebase knowledge required. P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. S: Needs Review Status: Requires additional reviews before being fully accepted T: Bugfix Type: Bugs and/or bugfixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Strange laser visual behaviour in open space
8 participants
@n00kii @Plykiya @Partmedia @slarticodefast @metalgearsloth @SaphireLattice @SlamBamActionman @TheShuEd