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

Space HVAC TGUI #17826

Merged
merged 24 commits into from Feb 9, 2024
Merged

Space HVAC TGUI #17826

merged 24 commits into from Feb 9, 2024

Conversation

IPling
Copy link
Contributor

@IPling IPling commented Feb 1, 2024

About the PR

Additions to the UI include:

  • Buttons to set to max temp, minimum temp and room temperature.
  • From temperature input to temperature slider
  • Changes temperature from Celsius to Kelvin
  • Fixes min temperature when emagged, should have been -120 C rather than 120 C
  • Removes some clarification errors. (Example: it used to say its non-emagged minimum was 20 C when in reality it was -90 C)
  • UI spice to when its emagged

space_hvac_showcase

Why's this needed?

Converting UI to TGUI is good.

Changelog

(u)IPingu
(+)Converts HVAC interface to TGUI

@boring-cyborg boring-cyborg bot added the A-UI Modifies UI in some way. Automatically applied on a change to tgui/ label Feb 1, 2024
@github-actions github-actions bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 1, 2024
Copy link
Contributor

@TobleroneSwordfish TobleroneSwordfish left a comment

Choose a reason for hiding this comment

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

Overall looks very good, more TGUI 👍

code/obj/machinery/spaceheater.dm Outdated Show resolved Hide resolved
code/obj/machinery/spaceheater.dm Outdated Show resolved Hide resolved
tgui/packages/tgui/interfaces/space_heater.js Outdated Show resolved Hide resolved
tgui/packages/tgui/interfaces/space_heater.js Outdated Show resolved Hide resolved
tgui/packages/tgui/interfaces/space_heater.js Outdated Show resolved Hide resolved
@Valtsu0
Copy link
Contributor

Valtsu0 commented Feb 1, 2024

I don't think the theme changing makes sense, especially with the background icon. But then again emagged things don't really need to make much sense so it's probably fine?

code/obj/machinery/spaceheater.dm Outdated Show resolved Hide resolved
tgui/packages/tgui/interfaces/space_heater.js Outdated Show resolved Hide resolved
@Azrun
Copy link
Contributor

Azrun commented Feb 1, 2024

I think the changelog should provide a little more detail on what has changed. "TGUI + heating improvements" makes me think that there would be minimal impact to behavior of the feature outside of porting the UI to TGUI. If this was just updating it to TGUI and you had some follow on PRs with the behavior changes that would be a lot easier to discuss.

Removing the requirement of screwdriving to access the UI

Why? Although it is a nice QoL improvement the in-ability to adjust the temperature conveys the sense that there is a "preferred" temperature and thus are set and forget.
sheater-open is no longer needed in the proposed implementation.

Changes input temperature from Celsius to Kelvin

Why? The scale of water seems more applicable for this being used to adjust the temperature of station to carbon based lifeforms.

Increases HVAC cooling and heating power in exchange for a higher power drain, which can have interesting affects when emagged. Allowing a room to heat up to the point certain chemicals ignite, as well possibly allowing a makeshift burn chamber.

Is there any particular reason you chose those constants and scale factors?

I think the the functionality of the proposed Space HVAC seems a bit more futuristic/extreme than what I think the current icon(s) convey.

@TobleroneSwordfish
Copy link
Contributor

Just for reference: this is why we say to split up UI reworks from content and balance changes, it can become review hell very easily 💀

@IPling
Copy link
Contributor Author

IPling commented Feb 2, 2024

I think the changelog should provide a little more detail on what has changed. "TGUI + heating improvements" makes me think that there would be minimal impact to behavior of the feature outside of porting the UI to TGUI. If this was just updating it to TGUI and you had some follow on PRs with the behavior changes that would be a lot easier to discuss.

Yeah I could atomize it into a nother PR, was just a bit too careless and thought it would be fine so long as I only changed its heating power, but I definitely added more than that. Altough I now got no clue how to actually atomize a PR so that is a problem.

Removing the requirement of screwdriving to access the UI

Why? Although it is a nice QoL improvement the in-ability to adjust the temperature conveys the sense that there is a "preferred" temperature and thus are set and forget. sheater-open is no longer needed in the proposed implementation.

Hiding the UI behind a screwdriver interaction was wierd, no other machine makes you screwdriver it open to access its UI and every machine has an on/off switch on their UI rather than having to physically interact with it. Context wise it didnt make sense, having to muddle around in the wires of the space hvac just to change the temperature settings.
'sheater-open' yeah I remember wanting to remove it, but I completely forgot about it and didnt notice it when making second-checks before making the PR, will discard it.

Changes input temperature from Celsius to Kelvin

Why? The scale of water seems more applicable for this being used to adjust the temperature of station to carbon based lifeforms.

In reality, yeah NT sending you a damn HVAC with a damn dial set to Kelvins makes little sense and is honestly quite funny. But the main reason is everything uses Kelvin instead of any other temperature measurement, so its better to exchange reality for less confusion and a little chuckle towards the staffie trying to operate the HVAC not understanding kelvins.

Increases HVAC cooling and heating power in exchange for a higher power drain, which can have interesting affects when emagged. Allowing a room to heat up to the point certain chemicals ignite, as well possibly allowing a makeshift burn chamber.

Is there any particular reason you chose those constants and scale factors?

For non-emagged it was so just the heater could actually warm and cool down a room, previously, it arguably could not without having to wait ages. The max and minimum were decided by ''How far can I go before people actually start dying from it.'', since it allows for uncomfortable temperatures for those people that ''Really enjoy a hot room.'' or vice versa.
For emagged, just set it to the max, it cannot actually reach 1k kelvins or below 200 kelvins an oxygenated room. It was arbitrary to set limits to emagging which was supposed to break them, the player can find the limitations from where they use it. Heating/cooling was just increased a buttload to allow the person to set the temp, walk away and a minute or two later the room is scorching hot.

I think the the functionality of the proposed Space HVAC seems a bit more futuristic/extreme than what I think the current icon(s) convey.

Definitely it seems a bit futuristic, but this thing is old, you can even tell by the DM name, the ''space_heater'' it wasnt meant to actually cool stuff down so the sprite doesnt exactly represent it. A blue flame represents it being cooled down, which makes no sense, but I aint a spriter so I dont have the power to change it.

@IPling
Copy link
Contributor Author

IPling commented Feb 2, 2024

Okay, tommorow gonna work on atomizing the mechanical changes of this PR so that it is freshly TGUI, since I definitely made a mistake of combining both TGUI and mechanical additions. I will PR the mechanical aspects after the TGUI has been merged and polished.

@IPling IPling changed the title Space HVAC TGUI + heating improvements Space HVAC TGUI Feb 3, 2024
@github-actions github-actions bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 3, 2024
@IPling
Copy link
Contributor Author

IPling commented Feb 3, 2024

Atomized anything related to mechanical changes and re-added the previous mechanics that existed. Will PR the mechanical changes after the TGUI has been merged.

@github-actions github-actions bot added the S-Merge-Conflict Applied and removed when a PR has or no longer has a merge conflict label Feb 4, 2024
@github-actions github-actions bot removed the S-Merge-Conflict Applied and removed when a PR has or no longer has a merge conflict label Feb 4, 2024
@Tarmunora
Copy link
Member

The theme changing thing is still kinda weird to have

@IPling
Copy link
Contributor Author

IPling commented Feb 5, 2024

The theme changing thing is still kinda weird to have

Is it really that bad? Personally I enjoy having more of a visual indication to what temp it is that isnt purely from the temperature slider. So I dont really understand what the problem is other than people saying ''Its wierd.''. There was the worry about the red syndicate logo in the background, but that is there because it was incredibly trivial to make a theme thats the same except with an NT logo in the back.

@Valtsu0
Copy link
Contributor

Valtsu0 commented Feb 5, 2024

I think the main reason why changing themes is weird is because this doesn't look like a screen, it looks like a physical control panel with a few buttons and a few small displays. Not only would it be weird for a physical panel to change color, it changes to themes that are more or less explicitly computer screens

@IPling
Copy link
Contributor Author

IPling commented Feb 5, 2024

I think the main reason why changing themes is weird is because this doesn't look like a screen, it looks like a physical control panel with a few buttons and a few small displays. Not only would it be weird for a physical panel to change color, it changes to themes that are more or less explicitly computer screens

Ah in that view it does make sense, I guess I can remove the global theme and make it local to the sections instead. That could give clarification to what is considered a screen and the physical body of the HVAC, plus it would remove the problem of the syndicate logo since I wouldnt be using themes. Altough I am not entirely sure what would be best to fix this confusion.

@IPling
Copy link
Contributor Author

IPling commented Feb 5, 2024

I changed it up a bit and did this (not yet commited), but I am not entirely sure if this exactly fixes the problem. Unless someone can come up with a good suggestion, I feel like this is a trivial problem.

space_hvac_color_change

@Valtsu0
Copy link
Contributor

Valtsu0 commented Feb 5, 2024

I like that

@Tarmunora
Copy link
Member

I changed it up a bit and did this (not yet commited), but I am not entirely sure if this exactly fixes the problem. Unless someone can come up with a good suggestion, I feel like this is a trivial problem.

space_hvac_color_change space_hvac_color_change

Temperature setting is looks to be still more-or-less unreadable when maxed out (red on red)

@IPling
Copy link
Contributor Author

IPling commented Feb 6, 2024

Hmm I guess I can commit it then if its more preferable, but yeah gonna fix that unreadable red text.

@IPling
Copy link
Contributor Author

IPling commented Feb 6, 2024

Updated the PR description to match the different look,

@github-actions github-actions bot added the S-Merge-Conflict Applied and removed when a PR has or no longer has a merge conflict label Feb 9, 2024
@github-actions github-actions bot removed the S-Merge-Conflict Applied and removed when a PR has or no longer has a merge conflict label Feb 9, 2024
@Tarmunora Tarmunora merged commit b4a9021 into goonstation:master Feb 9, 2024
22 checks passed
github-actions bot pushed a commit that referenced this pull request Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Modifies UI in some way. Automatically applied on a change to tgui/ size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants