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

Remove nukies axe plating prying capabilities. #1844

Merged
merged 4 commits into from
Sep 18, 2024

Conversation

Radezolid
Copy link
Contributor

About the PR

Removed the ability to pry plating as I think the intent behind #1700 was to make only the atmos (and bridge) axe being able to pry plating.

Why / Balance

Nukies are going back to the old meta of carrying the axe spacing every room they are in with it.

Technical details

I inverted the inheritance so the nukie fire axe doesn't get the Axing tag.

Media

Before

2024-09-17.02-05-55.mp4

After

2024-09-17.01-51-36.mp4

Requirements

Breaking changes

I hope none.

Changelog

🆑

  • fix: Nuclear operatives fire axe is no longer able to pry plating.

@Radezolid Radezolid requested a review from a team as a code owner September 17, 2024 05:12
@Radezolid
Copy link
Contributor Author

Insight on why the YAML Linter test failed would be appreciated. The part it complains about was not modified

Copy link
Member

@deltanedas deltanedas left a comment

Choose a reason for hiding this comment

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

you are completely mangling upstream files without even putting comments

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

  1. Deltanedas was straight to the point about it, but yes. Worth it to comment things and document what you have changed so that if there's suddenly a merge to add functionality to fireaxes it doesn't explode in the upstream merge

  2. The reason why the yaml linter is choking is because, if I had to guess with my currently tired brain, the fields for that aren't defined to go with the flaming fireaxe (the parents of that item are baseitem and base engi contra), and swapping the inheritance may not be this simple. It'd be easier to do this in a different way, but it might be more trouble than it's worth 🤷

@TadJohnson00
Copy link
Contributor

I'm gonna let Adei take the lead on trying to address the YAML side of all this. As for the Direction side, can't promise this is gonna sail, will get back to you on whether this'll go through or not.

@Radezolid
Copy link
Contributor Author

  1. Deltanedas was straight to the point about it, but yes. Worth it to comment things and document what you have changed so that if there's suddenly a merge to add functionality to fireaxes it doesn't explode in the upstream merge

    1. The reason why the yaml linter is choking is because, if I had to guess with my currently tired brain, the fields for that aren't defined to go with the flaming fireaxe (the parents of that item are baseitem and base engi contra), and swapping the inheritance may not be this simple. It'd be easier to do this in a different way, but it might be more trouble than it's worth 🤷

Thanks for the corrections, still learning this. If able what would be the different way? Make a new yml just for the nukie one?

I'm gonna let Adei take the lead on trying to address the YAML side of all this. As for the Direction side, can't promise this is gonna sail, will get back to you on whether this'll go through or not.

Sure thing, I was about to make a issue about the axe, but I tried my hand at it.

@deltanedas
Copy link
Member

can you not just do qualities: [] to disable it for fire axe

@Radezolid
Copy link
Contributor Author

can you not just do qualities: [] to disable it for fire axe

Tested it, worked perfectly, I should have done that. Thanks, as I said still learning this.

@ghost
Copy link

ghost commented Sep 18, 2024

can you not just do qualities: [] to disable it for fire axe

Tested it, worked perfectly, I should have done that. Thanks, as I said still learning this.

Was gonna suggest making an abstract fire axe and then parenting the atmos and nukie fire axes to this, but I guess this works, so well done.

@ghost
Copy link

ghost commented Sep 18, 2024

Deltanedas, unrequest your change and merge it, it's one of the things where I can't

@deltanedas deltanedas merged commit f44f68a into DeltaV-Station:master Sep 18, 2024
11 checks passed
@Radezolid Radezolid deleted the Fix-fireaxe branch September 18, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants