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

use unused filled chemmaster sprite #2729

Merged
merged 5 commits into from
Jan 19, 2025

Conversation

Temoffy
Copy link
Contributor

@Temoffy Temoffy commented Jan 19, 2025

About the PR

Now the chemmaster shows a different sprite when it has a beaker!

Why / Balance

unused sprite was languishing in the code, bring it in and warm it up

How to test

put beaker in chemmaster

Media

400129826-0d356d13-e62f-4dd9-b8cf-1460a87de8f5

Requirements

Breaking changes

Changelog

🆑

  • fix: Chemmaster now uses a new sprite when beaker inserted!

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.

Works a treat.

@Temoffy
Copy link
Contributor Author

Temoffy commented Jan 19, 2025

I see what you did and why, and I remember doing that when I originally did this in the chemprentice PR.

But I am extremely puzzled as to why it worked properly in my testing for the PR without that mapping in place

@Temoffy
Copy link
Contributor Author

Temoffy commented Jan 19, 2025

but that's a question for another time I suppose, even if it somehow autolinked due to the shared state reference, it is still best practice for clarity to specifically map it to a layer.

@whatston3
Copy link
Contributor

I see what you did and why, and I remember doing that when I originally did this in the chemprentice PR.

But I am extremely puzzled as to why it worked properly in my testing for the PR without that mapping in place

https://github.com/new-frontiers-14/frontier-station-14/pull/2646/files#diff-604436d7246b8b539040eb38cf34e81f4ebef31c04e29062487b9bb0185355a4R14

You seem to have had it here before, and this new branch didn't show the screen under test.

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.

Looks good to me.

@whatston3 whatston3 dismissed dvir001’s stale review January 19, 2025 22:46

Requested changes were made.

@whatston3 whatston3 merged commit 84f6940 into new-frontiers-14:master Jan 19, 2025
13 checks passed
FrontierATC added a commit that referenced this pull request Jan 19, 2025
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