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

Hotfix explosive bolt being craftable via anything with Comp Explosive #2976

Merged

Conversation

nathan736
Copy link
Contributor

@nathan736 nathan736 commented Feb 24, 2025

this included RTGS & Air tanks for some reason, moved the targeting requirement to a Firebomb tag. which might not be optimal but works.

About the PR

Why / Balance

Fixes a glaring issue reported in Bug reports
https://discord.com/channels/1123826877245694004/1343433692139225139/1343433692139225139

How to test

Attempt to create a explosive bolt with an RTG instead of the firebomb (reported to be able to be done with any explosive comp including airtanks)

Media

image
no longer able to be crafted with an RTG

Requirements

Breaking changes

Changelog

🆑

  • fix: Explosive bolts can now only be crafted with firebombs.

this included RTGS & Air tanks for some reason, moved the targeting requirement to a Firebomb tag. which might not be optimal but works.
@nathan736
Copy link
Contributor Author

nathan736 commented Feb 24, 2025

This is my First attempt of this kind so this Might be a very poor method to fix this
I am not Attatched to the tags name at all and there's likely a better name for it

@github-actions github-actions bot added size/S and removed size/XS labels Feb 24, 2025
@dvir001
Copy link
Contributor

dvir001 commented Feb 24, 2025

Convert to empty component, no new tag please.

@dvir001 dvir001 added S: Awaiting Changes This PR has changes that need to be made before merging S: DO NOT MERGE labels Feb 24, 2025
@whatston3
Copy link
Contributor

whatston3 commented Feb 24, 2025

This is my First attempt of this kind so this Might be a very poor method to fix this I am not Attatched to the tags name at all and there's likely a better name for it

Thanks for the contribution. It's reasonable, but I think dvir's right on this one.

The reason we don't like tags is that they always push complete inheritance - if you create a new tag and tag an item with only that (as you did here), you remove all previous tags (unsure if it's an issue in this particular case). If you look at Content.Server/_NF/Whitelist/Components, you'll see a bunch of empty [components] used for tagging items to avoid this issue.

A better longer-term solution would be to build support for an NFTag that can add and remove tags from a particular entity, rather than specifying a complete set, but that's a bit more involved.

@github-actions github-actions bot added the C# label Feb 24, 2025
@dvir001 dvir001 added S: Needs Review This PR is awaiting reviews and removed S: Awaiting Changes This PR has changes that need to be made before merging S: DO NOT MERGE labels Feb 24, 2025
@dvir001 dvir001 requested a review from whatston3 February 24, 2025 19:45
@dvir001 dvir001 self-requested a review February 25, 2025 11:53
Copy link
Contributor

@dvir001 dvir001 left a comment

Choose a reason for hiding this comment

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

Looking good.

/// Whitelist component for crafting explosive arrow bolts
/// </summary>
[RegisterComponent]
public sealed partial class NFExplosivecraftingComponent : Component;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we not put the comp in shared? @whatston3

Copy link
Contributor

@whatston3 whatston3 Feb 25, 2025

Choose a reason for hiding this comment

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

We could move all of the whitelist components into shared, and it's probably a good idea. At the moment, the only time this would get used is in a construction graph with a ghost, which we do not have.

Works as-is, but might break if construction graphs are moved to be more predicted.

Copy link
Contributor

@whatston3 whatston3 left a comment

Choose a reason for hiding this comment

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

Swapped the component name over from NFExplosivecrafting (vague, not camel case) to NFFireBomb. Works fine.

@whatston3 whatston3 removed S: Needs Review This PR is awaiting reviews S: Untriaged labels Feb 25, 2025
@dvir001 dvir001 self-requested a review February 25, 2025 16:44
@github-actions github-actions bot added the S: Needs Review This PR is awaiting reviews label Feb 25, 2025
@whatston3 whatston3 merged commit cc500bb into new-frontiers-14:master Feb 25, 2025
13 checks passed
FrontierATC added a commit that referenced this pull request Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants