rdiankov / openrave

Open Robotics Automation Virtual Environment: An environment for testing, developing, and deploying robotics motion planning algorithms.
http://www.openrave.org
Other
693 stars 343 forks source link

add backward compatibility for RaveOrientedBox deserialization #1280

Closed eisoku9618 closed 1 year ago

eisoku9618 commented 1 year ago

In the past, RaveOrientedBox was serialized with halfExtents / pose keywords.

rdiankov commented 1 year ago

I just noticed this. If possible, let's serialize to halfExtents, that is more standard

cielavenir commented 1 year ago

@eisoku9618 that scene was ok to load.

cielavenir commented 1 year ago

@eisoku9618 sorry, this pull request is blocking testplanningcommon. Please talk with @yoshikikanemoto (and @kanbouchou) for serialization format.

yoshikikanemoto commented 1 year ago

@rdiankov there was old scenes that use "pose" and "halfExtents" instead of "transform" and "extents". this change is just to support loading those old scenes.

cielavenir commented 1 year ago

@yoshikikanemoto so we will use "transform" and "extents" as default. Thank you for clarification.

eisoku9618 commented 1 year ago

If possible, let's serialize to halfExtents, that is more standard

Since many code use extents e.g. RaveOrientedBox / RaveAxisAlignedBox / PyOrientedBox / PyAABB so it takes some time to change all of them to halfExtents... :cry:

cielavenir commented 1 year ago

I agree that the C++ variable name and the serialization key should be the same.

rdiankov commented 1 year ago

Geometry already serializes to "halfExtents", so let's be consistent with the serialization, even if the variable names are not:

    case GT_Box:
        orjson::SetJsonValueByKey(rGeometryInfo, "halfExtents", _vGeomData*fUnitScale, allocator);
        break;
eisoku9618 commented 1 year ago

@rdiankov @cielavenir Sure, I changed to be consistent with the serialization at least.

Here is the situation with this MR.

eisoku9618 commented 1 year ago

test failed. checking.

eisoku9618 commented 1 year ago

@rdiankov Could you review this MR again? We also need to merge one internal repository together with this change.

Pipeline #579242

rdiankov commented 1 year ago

thanks~