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

HFR does not seem to fully observe delta_time #55983

Closed
esainane opened this issue Jan 6, 2021 · 5 comments · Fixed by #56009
Closed

HFR does not seem to fully observe delta_time #55983

esainane opened this issue Jan 6, 2021 · 5 comments · Fixed by #56009
Labels
Atmospherics Nobody knows how this code works Bug Correct Functionality

Comments

@esainane
Copy link
Contributor

esainane commented Jan 6, 2021

Opening for discussion, since there's quite a lot to go through. Please let me know if there's something obvious I'm missing!


Each fusion power level's var/scaled_production calculation can double-dip on delta_time whenever production is bounded by fuel injection, with unpredictable results:

var/fuel_consumption = clamp((fuel_injection_rate * 0.001) * 5 * power_level, 0.05, 30) * delta_time
var/scaled_production = clamp(heat_output * 1e-X, 0, fuel_consumption) * delta_time

A possible fix might be to split fuel_consumption into a fuel_consumption_rate, used for clamping purposes, and fuel_consumption, used for actually consuming fuel and producing Helium.


The fusion level 5 and 6 responses to Healium differ.

Level 5:

critical_threshold_proximity = max(critical_threshold_proximity - (m_healium / 100), 0)
moderator_internal.gases[/datum/gas/healium][MOLES] -= min(moderator_internal.gases[/datum/gas/healium][MOLES], scaled_production * 20)

Level 6:

critical_threshold_proximity = max(critical_threshold_proximity - (m_healium / 100), 0)
moderator_internal.gases[/datum/gas/healium][MOLES] -= min(moderator_internal.gases[/datum/gas/healium][MOLES], scaled_production * 20) * delta_time

Both provide a fixed amount of healing per update, based on the amount of Healium in the moderator. Level 6's response can also up to triple dip on delta_time when consuming Healium, since scaled_production can already use delta_time as a factor twice.


Level 5's Antinoblium production produces a fixed amount of Antinoblium per update, based on the proportion of Helium in the fusion mix:

internal_output.gases[/datum/gas/antinoblium][MOLES] += 0.1 * (scaled_helium / (fuel_injection_rate * 0.0065))

General moderator gas consumption removes each type of gas at a fixed proportion per update, based on power_level:

moderator_internal.remove(moderator_internal.total_moles() * 0.0005 * power_level)

Waste removal also ignores delta_time:

                if(filtering && moderator_internal.gases[filter_type])
                        var/datum/gas_mixture/removed = moderator_internal.remove_specific(filter_type, 20)
                        removed.temperature = moderator_internal.temperature
                        linked_output.airs[1].merge(removed)

                var/datum/gas_mixture/internal_remove
                if(internal_fusion.gases[/datum/gas/helium][MOLES] > 0)
                        internal_remove = internal_fusion.remove_specific(/datum/gas/helium, internal_fusion.gases[/datum/gas/helium][MOLES] * 0.5)
                        linked_output.airs[1].merge(internal_remove)
                if(internal_fusion.gases[/datum/gas/antinoblium][MOLES] > 0)
                        internal_remove = internal_fusion.remove_specific(/datum/gas/antinoblium, internal_fusion.gases[/datum/gas/antinoblium][MOLES] * 0.05)
                        linked_output.airs[1].merge(internal_remove)

Random radiation and hallucinations also ignore delta_time, if we should care about that.

@Dorsisdwarf Dorsisdwarf added Atmospherics Nobody knows how this code works Bug Correct Functionality labels Jan 6, 2021
esainane added a commit to esainane/tgstation that referenced this issue Jan 7, 2021
@LemonInTheDark
Copy link
Member

As I mentioned on the pr, inconsistencies in delta_time usage are bad, however I've given it some thought and I don't think we should be using it in SSair in the first place. I'll go into more detail on that now.

Everything in the subsystem is framed around interacting with flow, in this case the main issue is excited groups in particular.

Excited groups settle, or breakdown based on the highest amount of change inside them. If wait is raised, more gas would be moved from vents/scrubbers and such each tick, which would fuck with this behavior. The same happens if wait is lowered, since vents/scrubbers would attempt to meet the old output and cause earlier breakdowns.

The point of delta_time is also somewhat moot in this case, as atmos is practically built to take longer to run then its wait would suggest. What's important for the subsystem is that things happen in the same proportion each run, not that things happen at the same rate each second.

There is a caveat to this, using delta time for stuff that doesn't impact gas-movement is good, so things like burning when your insides are on fire, etc. Conveying that might be troublesome though.

@esainane
Copy link
Contributor Author

esainane commented Jan 8, 2021

As I understand it, and please correct me if I'm wrong on this or any other point, the bulk of HFR logic is located (directly or indirectly) from two main callins:

  • process_atmos from SSair. This mainly deals with injecting fuel and moderator gas from external pipenets, setting the internal fusion level based on the new temperatures calculated, etc.
  • process from SSmachinery. This mainly deals with the fusion reactions which were moved out of the atmos subsystem to allow for greater complexity.

The notes in this issue, and the changes made in PR #56009, exclusively deal with the logic in process_fusion. This is an internal proc called from the process callin, and uses the delta_time passed in from SSmachinery.

HFR's process_atmos callin does not use (or even list as an argument) the delta_time that SSair passes in process_atmos:

/obj/machinery/atmospherics/components/unary/hypertorus/core/process_atmos()

@LemonInTheDark
Copy link
Member

wait why is it doing gas things with process_fusion

@esainane
Copy link
Contributor Author

It's a bit harder to make normative rather than descriptive statements from just the code, but I think the intent was so that complicated reactions could happen on a slower update, and externally visible atmos changes would happen in process_atmos?

Would it be reasonable to merge #56009 for consistency now, and to make any broader redesign/refactor into a separate issue?

@LemonInTheDark
Copy link
Member

Oh of course, ever if the setup is jank, the pr is more then valid

esainane added a commit to esainane/tgstation that referenced this issue Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Atmospherics Nobody knows how this code works Bug Correct Functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants