-
Notifications
You must be signed in to change notification settings - Fork 37
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
ScanningGadget & KnownTech handling #473
ScanningGadget & KnownTech handling #473
Conversation
…olete The previous one was overriding the PDA scanner entry for the requirement tech type, which is against the rules of gadgets rules and very confusing.
The |
Fixed a bug where calling the `RemoveAllCurrentAnalysisTechEntry` would also remove the unlock for the target's analysis tech entry, resulting in unexpected behavior.
We now call the original `KnownTech.Initialize` method to ensure mods that patch the method don't get unnoticed upon repatching.
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.
It looks right and doesn't seem to break any of my mods from the small testing I did.
And while my gut still feels like somethings wrong here it seems to work, and I can't figure out what my gut is trying to tell me so.......
Since buildables are never registered to a crafting tree, Nautilus always logs a warning for that interpreting it as a mistake, when in reality it isn't.
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.
either way I think this is good enough.
Just wondering about setting that 1 log to Debug. if not then good, if yes then good, either way get this merged already lol
} | ||
else if (!prefab.TryGetGadget(out ScanningGadget scanningGadget) || !scanningGadget.IsBuildable) | ||
{ | ||
InternalLogger.Log($"Prefab '{prefab.Info.ClassID}' was not automatically registered into a crafting tree."); |
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.
Think we could make this Debug log instead?
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.
I think marking that as a debug log will make it easy to miss because a craftable item without a crafting tree that isn't a buildable is alarming and it's safer to assume that the developer missed something. In fact, I'd even argue it should be a warning instead.
Added an overload for `WithAnalysisTech` that doesn't take a story goal. This way it's easier to write the same code for both SN and BZ.
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.
LGTM
Changes made in this pull request
resolves #402
fixes #466
KnownTechPatcher
implementation; special thanks to @vlyon for the suggestion in AnalysisTech & Unlocking #402AddRequirementForUnlock
method that just adds blueprints to a requirement tech typeWithScannerEntry
overload for theScanningGadget
that doesn't override a techtype's scanner entry that it has no business inScanningGadget
now always adds analysis tech to the current tech type, when theWithAnalysisTech
method is calledunlockall
commandAnalysisTech
.Breaking changes
Deprecated methods
TODO:
KnownTechPatcher.RemovalTechs
efficiently