Also adds a skeletton of unittest file for it!
Details
Details
- Reviewers
Campbell Barton (campbellbarton) - Maniphest Tasks
- T38722: Adding units in Imperial setting results in inconsistent values
T39267: Implement UnitTesting for UI Units handling, and improve it - Commits
- rBSf94b87bbb89b: New python API for units handling.
rBf94b87bbb89b: New python API for units handling.
rBa63aa0524a2e: Modifications from Campbell's review, thanks! :)
rB061ea400a024: First step toward a basic units py API (exposes all supported unti systems &…
Diff Detail
Diff Detail
- Branch
- units_py_tests
Event Timeline
Comment Actions
Id still like to check on how the API should work a bit more...
C Unit API may eventually have its own conversion callbacks (rather then simply using a multiplier), The Python API should support this.
It looks like this should work with the current API, but just to say its a consideration.
Id like to do a second review, before applying this patch, just to test the API and see how it works exactly.
Left initial feedback inline...
| source/blender/python/intern/bpy_utils_units.c | ||
|---|---|---|
| 93 | you could use sizeof(_usystems) / sizeof(*_usystems) here. prefer not to hard code lengths here. | |
| 103 | types are typically bpy.types. bmesh.types - Python classes, looks like its different in this case, maybe this could be named less confusingly. | |
| 195 | prefer int *r_usys, int *r_utype for return args. | |
| 208 | prefer using STREQ macro. | |
| 252 | You can avoid a call to strlen(input) with "s#" arg to PyArg_ParseTupleAndKeywords | |
| 340 | *picky*, re: method names... not sure of best names... perhaps..
| |
Comment Actions
Updated against latest master, taking into account first review:
- Renamed unit types to categories.
- Switched func names to to_value/to_string.
- Other more minor changes and cleanup.
Comment Actions
Thanks for first review, Campbell, and of, there is no need to rush here, better do things nice :)
Regarding C api, since this code uses it intensively, I think it should not be a big issue is C one extends…
| source/blender/python/intern/bpy_interface.c | ||
|---|---|---|
| 573 | differentiation here is a bit fuzzy.
bpy_button_exec Should probably be moved to generic api py_capi_utils.c, maybe call. PyC_RunString_AsNumber(const char *expr, double *value, const char *filename) {...}Then call as: PyC_RunString_AsNumber(expr. &value, "<blender button>") | |
| source/blender/python/intern/bpy_utils_units.c | ||
| 64 | prefer not _*** prefix here (typically only for function local, private vars). Pick some generic Prefix? - bpyunits_ for eg? | |
| 97 | ARRAY_SIZE can be used here. | |
| 174 | *picky*, would rather make this a function, BLI_string_index_in_array()? | |
Comment Actions
Committed some changes to temp-pyapi-units,
Currently the test fails for me,
test.support.TestFailed: Traceback (most recent call last):
File "/src/blender/source/tests/bl_pyapi_units.py", line 23, in test_units_inputs self.assertAlmostEqual(units.to_value(usys, utype, inpt, ref), val)
AssertionError: 1.3048 != 0.6096 within 7 places
| source/blender/python/intern/bpy_utils_units.c | ||
|---|---|---|
| 413 | Why cant this be a module? Adding new PyTypes has a bit of overhead, and from what I can see this would work perfectly well as s module. see BPY_rna_props for simple example. | |
Comment Actions
Regarding test failing, this is on purpose currently (it shows a situation which fails with current code, when you write 1+1ft, you would expect to get 2 feet, not 1BU + 1foot - which is precisely the purpose of D340 ;) ). That test should be commented before going into master, and re-enabled after D340 goes in too.
| source/blender/python/intern/bpy_utils_units.c | ||
|---|---|---|
| 413 | Eeeeh… not sure… I just made same thing as e.g. bpy.app.translations… Will try to make it a sub-module, then. | |