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

Fixed additional texture rotation issue. See Issue #2737. #4515

Closed
wants to merge 7 commits into from

Conversation

dwhipps
Copy link
Contributor

@dwhipps dwhipps commented Oct 24, 2016

Fixes remaining texture rotation issue #2737 .

@hpinkos
Copy link
Contributor

hpinkos commented Oct 25, 2016

@dwhipps I don't think this is the right solution.
I was testing with this code:

var viewer = new Cesium.Viewer('cesiumContainer');

var rectangle = Cesium.Rectangle.fromDegrees(-86.69777, 30.03883, -76.69777, 40.03883);
var stRotation = Cesium.Math.toRadians(0.0);
var rotation = Cesium.Math.toRadians(30);
viewer.entities.add({
    rectangle: {
        coordinates: rectangle,
        material: '../images/Cesium_Logo_Color.jpg',
        rotation: rotation,
        stRotation: stRotation
    }
});

This is what I see in master:
image

And this is what I see on your branch:
image

@dwhipps
Copy link
Contributor Author

dwhipps commented Oct 25, 2016

Hmm. Fair enough. That doesn't look right. I'll check it out.

@dwhipps
Copy link
Contributor Author

dwhipps commented Oct 26, 2016

Hannah,

Try this (notice the very small but non-zero texture rotation):

var viewer = new Cesium.Viewer('cesiumContainer');

var rectangle = Cesium.Rectangle.fromDegrees(-86.69777, 30.03883, -76.69777, 40.03883);
var stRotation = Cesium.Math.toRadians(0.0000000001);
var rotation = Cesium.Math.toRadians(30);
viewer.entities.add({
    rectangle: {
        coordinates: rectangle,
        material: '../images/Cesium_Logo_Color.jpg',
        rotation: rotation,
        stRotation: stRotation
    }
});

@dwhipps
Copy link
Contributor Author

dwhipps commented Oct 26, 2016

@hpinkos @jorpiell @pjcozzi Why would there be such a drastic difference between stRotation = 0.0 and stRotation ALMOST 0.0? I can't see anywhere Cesium is checking for stRotation == 0.0 or where we might be making the mistake of e.g.

if (stRotation) instead ofif (defined(stRotation))

Anyone? I feel like I'm pretty close here, but could use another pair of eyes on this.

@dwhipps
Copy link
Contributor Author

dwhipps commented Oct 26, 2016

@hpinkos Do you think this might be related to your fix in #4206 ? This seems to be another instance where the rotation (in this case the texture rotation stRotation) is zero and somehow breaks things.

@jorpiell
Copy link

Both @hpinkos and @dwhipps are right: in master, the rotation with the angle 0.0 doesn't fails because the pull request that @dwhipps did has not been pushed yet. But in master, the rotation problem has not been fixed yet.

Have a look to the pull request #4535. It includes the minimum changes that work with all the previous use cases and it also works when the angle is 0.

… feature/texture-rotation

Keeping up to date with master.
@dwhipps
Copy link
Contributor Author

dwhipps commented Nov 2, 2016

@hpinkos I think we need someone at AGI to take the reins on this one. Some combination of the pull requests should finish this off. Is there someone there who can do that? I'm not sure how else I can contribute.

@hpinkos
Copy link
Contributor

hpinkos commented Nov 2, 2016

Thanks @dwhipps, I'll take a look at this and @jorpiell's PR when I have a chance. I'm in the middle of something else so it may be a little bit, but I'll try to get to this soon.

Keeping up to date with master branch
@dwhipps
Copy link
Contributor Author

dwhipps commented Nov 28, 2016

@hpinkos Any further thoughts on this? Technically this is still broken on master branch.

@hpinkos
Copy link
Contributor

hpinkos commented Dec 7, 2016

@dwhipps I submitted a fix for this in #4725. Check it out and let me know if it's working for you!
Thanks

@hpinkos hpinkos closed this Dec 7, 2016
@dwhipps
Copy link
Contributor Author

dwhipps commented Dec 7, 2016

@hpinkos Looks good! Thanks.

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

Successfully merging this pull request may close these issues.

None yet

3 participants