Added keyword arguments to createConstraint .
Changed initial values for the pivod XYZ form 1 to 0.0.
Changed initial values for the axis Z form 1 to 0.0.
Delete the parsing for 4 parameters, because parsing only the X pivot is not necessary, also it was not working correctly (int instead of float) .
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Actually the [pivotX] which was using the variable extrainfo was never working correct because it is an integer and need to be an float.
I think this was the attempt to set only the parameters that are necessary for the line hinge constraint.
We can change the extrainfo to an float to get this option working or we make all more consistent and use the fourth parameter for the linked collision flag.
We can also add five parameter version which will get createConstraint(physicsid, physicsid2, constrainttype, [pivotX], [flag]). Actually missing in the patch.
But actually I have a problem to proper document the parameters. How we should document all the parameter options correctly?
createConstraint(physicsid, physicsid2, constrainttype, [flag])
createConstraint(physicsid, physicsid2, constrainttype, [pivotX, [flag]])
createConstraint(physicsid, physicsid2, constrainttype, [pivotX, pivotY, pivotZ, [flag]])
createConstraint(physicsid, physicsid2, constrainttype, [pivotX, pivotY, pivotZ, [axisX, axisY, axisZ, [flag]]])
Or more like this?
createConstraint(physicsid, physicsid2, constrainttype, [pivotX], [pivotY, pivotZ], [axisX, axisY, axisZ], [flag])
Comment Actions
The functionality seems ok to me.
You need to update to current master because after commit rB9bd2a7c0a8ff there is no need for #if define(_WIN64). Moreover, the new format should be "KKii" / "KKifffi".
Regarding to the documentation I have no strong preference.
Comment Actions
IMO axisX, axisY, axisZ should just be passed as a 3D vector axis instead.
It could also be a nicer idea to pass pivotX, pivotY, pivotZ as one parameter pivot too. The type (float, 2D vector or 3D vector) then determines the type of constraint. This will also make the documentation easier to read.
Disclaimer: this is just me looking at this method with fresh eyes. Breaking any form of compatibility is not something to be done lightly.
Comment Actions
I talked with moguri he met we should not break compatibility for now.
Also the most would initialize the parameters directly. so there is not much improvement if we add an vector type to this function.
createConstraint(physicsid, physicsid2, constrainttype, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 128) --> createConstraint(physicsid, physicsid2, constrainttype, [0.0, 0.0, 0.0], [0.0, 0.0, 0.0], 128)
I decided to add keyword arguments to to the function, now it is possible to change only one optional parameter.
Eg. createConstraint(obj1_ID, obj2_ID, constraintType, pivot_x = 1.0, pivot_z = -1.0, axis_z = 90.0, flag = 128)
About the 4 parameter. I think deleting the parsing of the 4. parameter don't brake the compatibility because it was never working correctly before.
Comment Actions
Other than the little comment it's ok for me.
| source/gameengine/Ketsji/KX_PyConstraintBinding.cpp | ||
|---|---|---|
| 510 | Would it be better "if ((len != 3) || (len != 6) || (len != 9) || (len != 10))"? this way we check for len < 3 and len > 10 also | |
| source/gameengine/Ketsji/KX_PyConstraintBinding.cpp | ||
|---|---|---|
| 510 | That is not necessary, because the other checks will be done by PyArg_ParseTupleAndKeywords method, which generates more advanced error messages. I don't want to overwrite it. | |
| source/gameengine/Ketsji/KX_PyConstraintBinding.cpp | ||
|---|---|---|
| 510 | Ok. | |
Comment Actions
This part concerns me a little:
Changed initial values for the pivod XYZ form 1 to 0.0.
Changed initial values for the axis Z form 1 to 0.0.
Could this break existing games?
Comment Actions
Yes I know. But I think it is better to fix that. Moving the XYZ pivot to 1,1,1 and rotate the Z axis at 1° make no sense to me.
If we set all to zero we have the same standard values as the Blender rigid body joint constraint.
Also the most users that are using the constrains API always set the pivot in here scripts. I think there is only a little percentage where pivot point at 1,1,1 have not to be changed.
So the most scripts should still work.
I will mention that possible compatibility issue in the Blender wiki.
Comment Actions
Theres some redundant code which can go, otherwise looks fine.
| source/gameengine/Ketsji/KX_PyConstraintBinding.cpp | ||
|---|---|---|
| 510 | As far as I can see this length check isnt needed. Original code that checks this is only doing so because I guess the author didnt know to use | in PyArg_ParseTuple | |
| 540–543 | Normally we use 8 spaces indentation for function args. Helps differentiate with indentation between braces. | |
| source/gameengine/Ketsji/KX_PyConstraintBinding.cpp | ||
|---|---|---|
| 540–543 | Then the Wiki is wrong.http://wiki.blender.org/index.php/Dev:Doc/Code_Style#Indentation.
| |
Comment Actions
Removed length check for PyArg_ParseTuple.
Replaced tab with 8 spaces indentation for function args.