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

offshore gas mining refactor #2750

Merged
merged 6 commits into from
Jan 21, 2025

Conversation

MilonPL
Copy link
Contributor

@MilonPL MilonPL commented Jan 20, 2025

About the PR

refactor of #2403
UI cleanup, making comments good, removing frontier comments from frontier files, namespacing everything correctly, using vars rather than specifying the type, entity pattern, predicted examine events and stuff, switched to primary constructors as those are preferred now, etc etc

Why / Balance

image

How to test

>look at what the original PR does
>still does the same thing

Requirements

Breaking changes

Changelog

no fun

@whatston3
Copy link
Contributor

>bait milon into code improvements
>it works
>text didn't turn green

Thanks for the code review.

@dvir001
Copy link
Contributor

dvir001 commented Jan 20, 2025

Your skibidi YAML linter aint got the rizz boi, not very sigma.

@whatston3 whatston3 mentioned this pull request Jan 20, 2025
2 tasks
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.

Generally looks fine. There's a few unused files in there that should've been destroyed, and I think the RandomGasDeposit should be separated out from the regular GasDeposit - the client really doesn't need to know about the deposit generation prototypes.

Content.Client/_NF/Atmos/UI/ShuttleGaslockControl.xaml.cs Outdated Show resolved Hide resolved
Comment on lines +79 to +82
private void OnExtractorMapInit(Entity<GasDepositExtractorComponent> ent, ref MapInitEvent args)
{
UpdateAppearance(ent);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on the ChemMaster's generic update function approach?

            SubscribeLocalEvent<GasDepositExtractorComponent, MapInitEvent>(SubscribeUpdateAppearance);
...
        private void SubscribeUpdateAppearance<T>(Entity<GasDepositExtractorComponent> ent, ref T ev)
        {
            UpdateAppearance(ent);
        }

Content.Server/_NF/Atmos/Systems/GasDepositSystem.cs Outdated Show resolved Hide resolved
@github-actions github-actions bot added the YML label Jan 20, 2025
@whatston3
Copy link
Contributor

Fixed issues with the inward/outward appearance not being used, unanchored the portable gaslock/mining drill by default. Separated out the RandomGasDeposit from the deposit itself.
Unneeded files removed.
Gas deposit anchor message predicted.
"deposit locator" renamed to "gas deposit locator".

Looks good to me.

@MilonPL
Copy link
Contributor Author

MilonPL commented Jan 21, 2025

Changes look good to me.

@Cheackraze Cheackraze merged commit 7f12f13 into new-frontiers-14:master Jan 21, 2025
13 checks passed
@MilonPL MilonPL deleted the dvir-smells-really-bad branch January 21, 2025 08:27
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.

4 participants