Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Matrix4.getRotation returns rotation with scale #7398

Closed
Vineg opened this issue Dec 10, 2018 · 4 comments · Fixed by #8182
Closed

Matrix4.getRotation returns rotation with scale #7398

Vineg opened this issue Dec 10, 2018 · 4 comments · Fixed by #8182

Comments

@Vineg
Copy link
Contributor

Vineg commented Dec 10, 2018

It seems to be misleading, since there is at least one place in codebase where Matrix4.getRotation is used as rotation-only. For example it leads to dim lighting of scaled point clouds, since normals are scaled as well. I think this place requires carefull review of existing usages and maybe change the name or normalize result by scale.

var viewer = new Cesium.Viewer('cesiumContainer');
viewer.clock.currentTime = new Cesium.JulianDate(2457522.154792);

var tileset = new Cesium.Cesium3DTileset({
    url: '../../SampleData/Cesium3DTiles/PointCloud/PointCloudNormals/tileset.json'
});
viewer.scene.primitives.add(tileset);
viewer.zoomTo(tileset, new Cesium.HeadingPitchRange(0.0, -1.0, 50.0));
var style = {};
style.pointSize = Cesium.defaultValue(style.pointSize, 5.0);
tileset.style = new Cesium.Cesium3DTileStyle(style);
Cesium.Matrix4.setScale(tileset.modelMatrix, new Cesium.Cartesian3(3,3,3), tileset.modelMatrix);

image

@Vineg
Copy link
Contributor Author

Vineg commented Dec 10, 2018

this is what expected
image

Vineg pushed a commit to geoscan/cesium that referenced this issue Dec 10, 2018
removed geometricError recalculation on each "get"
@hpinkos
Copy link
Contributor

hpinkos commented Dec 10, 2018

Thanks for opening up the issue @Vineg! This seems like a clear cut bug.

@lilleyse can you look into this?

@lilleyse
Copy link
Contributor

I've been confused by this in the past too. It seems like Matrix4.getRotation has always just returned the upper left 3x3 matrix, which may include scale.

Should Matrix4.getRotation be deprecated in favor of Matrix4.getMatrix3 or Matrix3.fromMatrix4?

@Vineg
Copy link
Contributor Author

Vineg commented Dec 10, 2018

Matrix4.getMatrix3 seems nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants