Page MenuHome

Create new Temperature unit
Needs ReviewPublic

Authored by Vaishnav S (padthai) on Feb 24 2019, 9:56 AM.



Made a new temperature unit and assigned it to flame_max_temp and flame_ignition. Also fixed a few typos.

Diff Detail

Event Timeline

Great! Thanks for this. And well done for supporting both Celsius and Fahrenheit depending on the Metric/Imperial setting.

This revision is now accepted and ready to land.Feb 25 2019, 11:13 AM

Generally looks good, but there are still a few things to consider...


Thanks for those typo fixes. I'll commit those separately.


I wonder if we should use oC or just C. The latter is more comfortable to type.


Unfortunately, the conversion is not that simple. Currently switching between the unit systems changes the absolute value of the temperature, i.e. it thinks 5C = 5F.

A bit more work has to be done to get this conversion working, because it is different from all the other conversions we have so far.

Also there is a missing white space.


It's probably a good time to split this into multiple lines now, one line per unit collection.


As always we have to check if the number actually resembles the unit somewhat correctly.

I don't know what this setting does exactly, but having a flame_mask_temp in the range from 1 - 10 does not make much sense, does it?

Similarly for the other property.

Jacques Lucke (JacquesLucke) requested changes to this revision.Feb 25 2019, 11:37 AM
This revision now requires changes to proceed.Feb 25 2019, 11:37 AM

Sorry for taking time on this one.
The celsius value gets converted to fahrenheit properly now using the bias variable.
About the indentation thing, I just can't get the diff to remove those whitespaces. The actual code looks fine I think.
Changed the temperature ranges for ignition temperature and max temperature from 1 - 3000; oxyacetylene torches can get upto 3000 C.


Increasing the max values is one part. The more difficult thing is to make sure that the set temperature is actually interpreted directly by the simulation solver...


Please try to keep these fixes separate from the patch. You can also just make separate very small patches for this stuff (the same is true for the typo fix at the top).

ah sorry about that. i'll update the diff and push a new patch.

Removed the typo changes. D4418 has the changes now.