Details

Reviewers

William Reynish (billreynish)
Jacques Lucke (JacquesLucke)
Brecht Van Lommel (brecht)
Vaishnav S (padthai)

Group Reviewers

User Interface

Maniphest Tasks

T60824: List of Properties that should use the Units system

Commits

rB36aeb0ec1e2e: UI: Add temperature units

Summary

Based on the original patch by @Vaishnav S (padthai), this adds support for temperature units. Initially supported units are Celsius, Kelvin, and Fahrenheit.

The units aren't actually used anywhere, those changes should happen in separate patches, as it should be ensured that the various solvers and simulations
actually properly use the units.

The complexity of some of the changes comes from the fact that these units have offsets from each other as well as coefficients. This also makes
the implementation in the current unit system a bit troublesome. For example, entering 0 C evaluates correctly to 273 K, but 0C + 0C doubles
that result, because each unit value is evaluated separately. This is quite hard to solve in the general case with Blender's current unit system, and
I think it's a quite niche problem anyway. It is unfortunate though.

Diff Detail

Repository: rB Blender

Event Timeline

Vaishnav S (padthai) created this revision.Feb 24 2019, 9:56 AM

Vaishnav S (padthai) added a task: T60824: List of Properties that should use the Units system.

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

William Reynish (billreynish) awarded a token.Feb 25 2019, 11:14 AM

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

source/blender/blenkernel/intern/unit.c
184	Thanks for those typo fixes. I'll commit those separately.
320	I wonder if we should use `oC` or just `C`. The latter is more comfortable to type.
326	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.
334	It's probably a good time to split this into multiple lines now, one line per unit collection.
source/blender/makesrna/intern/rna_smoke.c
772 ↗	(On Diff #13820)	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.

Jacques Lucke (JacquesLucke) added inline comments.Feb 27 2019, 10:57 AM

source/blender/makesrna/intern/rna_smoke.c
772 ↗	(On Diff #13820)	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...
source/blender/python/intern/bpy_props.c
133	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.

William Reynish (billreynish) added a reviewer: User Interface.May 9 2019, 3:51 PM

Hans Goudey (HooglyBoogly) added a project: User Interface.Jun 25 2020, 4:12 PM

Update the patch to the current state of master

I actually removed the changes to the smoke properties for now. Actually using temperature
units can be a separate improvement. As @Jacques Lucke (JacquesLucke) mentions, it may actually require
changes to the simulation solvers.

@Vaishnav S (padthai) I hope it's okay that I updated this patch. I was cleaning out old revisions and
noticed this one that I believe we should finish up.

Harbormaster completed remote builds in B9831: Diff 28213.Aug 26 2020, 9:26 PM

Looks good to me, but better wait for approval from Brecht.

source/blender/blenkernel/intern/unit.c
84	You can add a couple more 5s here.

This revision is now accepted and ready to land.Aug 26 2020, 9:35 PM

Add all the fives

Harbormaster completed remote builds in B9832: Diff 28214.Aug 26 2020, 9:43 PM

For fire simulation, rendering of blackbody emission or color temperature, it's more common in computer graphics to use Kelvin.

There may be other cases where Celsius or Fahrenheit is a better default, though I can't think of existing properties in Blender. If we do need them probably we'd need two separate temperature units.

So by itself this patch is fine with me. But if the purpose is to add temperature to existing Blender properties, there should be a temperature unit that stores Kelvin internally and also default to it in the UI.

Kelvin units, initial UI support

This turned out to be much more complicated than I expected because the existing unit evaluation
code did not handle the bias value necessary to offset Kelvin, Celsius, and Fahrenheit from each
other.

There are some unfortunate side effects of the way the unit system works here. For example, evaluating
0C + 0C + 0C will give 819 K because the intermediate interperetation is (0+273.15)*1#+(0+273.15)*1#+(0+273.15)*1#.
While fixing this is technically possible, Blender's unit system isn't perfect already, and it probably
shouldn't be high priority to make it so. Still, it doesn't feel great.

A couple more things to do: Make sure farhenheit works correctly and add support for entering numbers
without units when celsius is the active system.

Harbormaster completed remote builds in B9907: Diff 28389.Sep 1 2020, 3:22 AM

This patch includes some cleanup that I'll pull out to a separate commit too.

Hans Goudey (HooglyBoogly) retitled this revision from Create new Temperature unit to UI: Add temperature units.Sep 1 2020, 3:42 AM

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)

I haven't looked at the implementation in detail, I'll leave that for @Jacques Lucke (JacquesLucke) to review.

release/scripts/startup/bl_ui/properties_scene.py
96	Is `temperature_test` debug code?
source/blender/blenkernel/intern/unit.c
343	I think Kelvin should also be the base unit in imperial? Something like color temperature you would still want to see as Kelvin even if you use imperial length units.

Hans Goudey (HooglyBoogly) added inline comments.Sep 1 2020, 7:32 PM

release/scripts/startup/bl_ui/properties_scene.py
96	Yep, just to have a button to test with.
source/blender/blenkernel/intern/unit.c
343	I was thinking about that too, I'll add that.

Support entering units with bias without specifying the unit
Add comment

This revision is now accepted and ready to land.Sep 2 2020, 12:16 AM

Harbormaster completed remote builds in B9929: Diff 28431.Sep 2 2020, 12:16 AM

Also init temperature unit for new scenes

Harbormaster completed remote builds in B9937: Diff 28453.Sep 2 2020, 5:19 PM

For example, entering 0 C evaluates correctly to 273 K, but 0C + 0C doubles that result, because each unit value is evaluated separately.

Does this just affect the one case or are all math operations affected with units other than kelvin? E.g. does 5C + 2C equal 280K/7C or does it equal 553K/280C?

Looks good. Just some minor changes are necessary.

It's a bit sad that the text manipulation is so hacky, but we can't/shouldn't change that in the same patch. So that can be done later if it is important enough.

source/blender/blenkernel/BKE_unit.h
50	That's a weird way to continue the sentence in the comment. Might be better to just have one function that returns both. Then you also cannot forget to cal one or the other.
source/blender/blenkernel/intern/unit.c
880	Based on the name, I'd expect this function to compare chars to digits, but that does not seem to be what it is doing.
934	typo
944	typo

Does this just affect the one case or are all math operations affected with units other than kelvin? E.g. does 5C + 2C equal 280K/7C or does it equal 553K/280C?

Yeah, unfortunately all operations. Even... multiplication? Because somehow we can do that with temperatures.
You can enter something like 1m * 3.1km * 80feet - 180.74cm * 18nm and it will be really happy to evaluate that for you as a length value.
For years entering -1m50cm would evaluate to -0.5, I only recently fixed that a few months ago.

The point is, the unit system would need quite a bit of work to get it to the point where it can consider groups of unit values together, and there probably are more pressing things to work on.

source/blender/blenkernel/intern/unit.c
880	I used "value" instead for both the helper functions. Hopefully that's more clear.

Review update: combine applying scalar and bias into one function, fix typos, rename functions

Harbormaster completed remote builds in B9968: Diff 28509.Sep 3 2020, 6:42 PM

Does this just affect the one case or are all math operations affected with units other than kelvin? E.g. does 5C + 2C equal 280K/7C or does it equal 553K/280C?

Yeah, unfortunately all operations. Even... multiplication? Because somehow we can do that with temperatures.

Lol. I figured as much. And yes, I remember that fix, nice one!

It's probably out of scope for this patch, but could you disallow math operations for inputting temperature? I doubt it's really needed because the change is so great that it would probably be easily caught, but I thought I'd mention it anyway.

Thanks for your improvements to the unit system!

Closed by commit rB36aeb0ec1e2e: UI: Add temperature units (authored by Hans Goudey (HooglyBoogly)). · Explain WhySep 7 2020, 9:59 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rB36aeb0ec1e2e: UI: Add temperature units.

UI: Add temperature units
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 28633

release/scripts/startup/bl_ui/properties_scene.py

source/blender/blenkernel/BKE_unit.h

source/blender/blenkernel/intern/scene.c

source/blender/blenkernel/intern/unit.c

source/blender/editors/util/numinput.c

source/blender/makesdna/DNA_scene_types.h

source/blender/makesrna/RNA_types.h

source/blender/makesrna/intern/makesrna.c

source/blender/makesrna/intern/rna_rna.c

source/blender/makesrna/intern/rna_scene.c

source/blender/python/intern/bpy_props.c

UI: Add temperature unitsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 28633

release/scripts/startup/bl_ui/properties_scene.py

source/blender/blenkernel/BKE_unit.h

source/blender/blenkernel/intern/scene.c

source/blender/blenkernel/intern/unit.c

source/blender/editors/util/numinput.c

source/blender/makesdna/DNA_scene_types.h

source/blender/makesrna/RNA_types.h

source/blender/makesrna/intern/makesrna.c

source/blender/makesrna/intern/rna_rna.c

source/blender/makesrna/intern/rna_scene.c

source/blender/python/intern/bpy_props.c

UI: Add temperature units
ClosedPublic
Actions

Revision Contents
Changeset List