Page MenuHome

First step toward a basic units py API (exposes all supported unti systems & types, and str_to_value/value_to_str functions).
ClosedPublic

Authored by Bastien Montagne (mont29) on Mar 19 2014, 4:32 PM.

Diff Detail

Event Timeline

Bastien Montagne (mont29) updated this revision to Unknown Object (????).Apr 27 2014, 8:21 PM

Updated against latest master.

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..

  • encode / decode ?
  • to_string / to_value ?
  • value_from_string / value_to_string ?
Bastien Montagne (mont29) updated this revision to Unknown Object (????).Apr 28 2014, 3:31 PM

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.

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…

Bastien Montagne (mont29) updated this revision to Unknown Object (????).Jun 16 2014, 9:29 AM

Updated against latest master.

Campbell Barton (campbellbarton) requested changes to this revision.Jun 16 2014, 3:17 PM
Campbell Barton (campbellbarton) added inline comments.
source/blender/python/intern/bpy_interface.c
573

differentiation here is a bit fuzzy.

  • bpy_button_exec - caller needs to handle errors, manage gil.
  • BPY_button_exec - called from UI, handle errors.

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()?

Bastien Montagne (mont29) updated this revision to Unknown Object (????).Jun 16 2014, 4:59 PM
  • Modifications from Campbell's review, thanks! :)

Wtf… Stupid robot! :P

Campbell, so, as suggested, temp branch: temp-pyapi-units.

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.

Campbell Barton (campbellbarton) requested changes to this revision.Jun 17 2014, 7:01 AM

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.