Page MenuHome

Fix T38722: Adding units in Imperial setting results in inconsistent values
ClosedPublic

Authored by Bastien Montagne (mont29) on Feb 22 2014, 11:37 AM.

Diff Detail

Repository
rB Blender

Event Timeline

Rather postpone this for 2.70. But looks like it could be a good fix too.

Should the addition of ')' to ch_is_op be added separately? looks like it may apply even without any of the other changes.

Bastien Montagne (mont29) updated this revision to Unknown Object (????).Mar 16 2014, 10:08 AM

Updated patch, changed some things (replaced ',' by '+' to separate units entities, else things like e.g. (1'1")*2.5 fails).

Talked to @Bastien Montagne (mont29),
This code is a little fragile and hard to guess how changes may impact other areas so proposed to...

  • Write py api for units bpy.utils.units
  • Collect a list of all expressions and their expected results. (even ones that currently fail)
  • Write unit tests similar to source/tests/bl_pyapi_mathutils.py

Then review and apply patches, making sure they don't make breaking changes to existing functionality.

This isn't urgent and Im happy to do this too, but @Bastien Montagne (mont29) is interested to check on getting this setup.

Bastien Montagne (mont29) updated this revision to Unknown Object (????).Jun 18 2014, 8:02 PM

Updated against latest master.

Note current tests defined for new 'units' pyapi all pass OK, need to add more of those
though...

Note: re-reading this patch, it still misses some important points imho… will rework it a bit.

Bastien Montagne (mont29) updated this revision to Unknown Object (????).Jul 17 2014, 10:28 AM

Updated against latest master, tweaked a bit things, added a few test cases.

Basically, this patch will try to find a default unit in current system, either from given string,
given previous string, or fallback to system's default unit, and will use this default unit
for all items of the addition that do not have an explicit one.

When found, default unit is always the biggest one available.

In other words:

1+1"    -> 2"
1+1"+2' -> 3'1"

This has the main benefit of keeping non-specified values in current unit system. Else,
typing 1+1" would give 1BU+1in, which is especially bad in imperial system...

Note: we could try to be even smarter, and infer 'implicit parenthesis' (as in 1+1"+2',
which would translate as (1+1)"+2'), but this would really complexify the code, and I
doubt we could always get expected result/behavior anyway, so imho better to force user
to use explicit parenthesis in those cases!

source/blender/blenkernel/intern/unit.c
661

*picky*, With this I worry that it may be haphazard which unit is found...

Would rather have some logic here...

  • Use largest unit (in that case search should be done in reverse order), or...
  • Use first unit referenced in string (search for all units and pick the one that shows up first)
source/blender/blenkernel/intern/unit.c
661

Well, it does select the biggest (largest) unit, just like code above? We first search for km, then m, then cm, then mm, etc.

I sit corrected :), other minor feedback.

overall would really like to check this better though...

source/blender/blenkernel/intern/unit.c
640

odd name. better call scale_pref_base / scale_pref_default ?

651

*picky*, previously this was isolated in its own block,

could move this into own static func? unit_detect_from_str

This revision is now accepted and ready to land.Aug 20 2014, 10:38 AM