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.

Sign up for GitHub

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

Jump to bottom

OrthographicFrustum deprecated properties are removed. #5170

Merged
merged 8 commits into from
Apr 28, 2017
Merged

OrthographicFrustum deprecated properties are removed. #5170

merged 8 commits into from
Apr 28, 2017

Conversation

ateshuseyin
Copy link
Contributor

fixes #5109

@hpinkos
Copy link
Contributor

hpinkos commented Mar 31, 2017

Thanks @ateshuseyin! We don't want to merge this until the 1.33 release, so we'll wait until after the 1.32 release goes out on Monday.

@bagnell does this look good to you?

@bagnell
Copy link
Contributor

bagnell commented Mar 31, 2017

Can you also remove all references to the _useDeprecated property as well as the required deprecationWarning? Otherwise, this looks good.

emackey
emackey reviewed Apr 11, 2017
View reviewed changes
CHANGES.md Outdated
@@ -32,6 +32,7 @@ Change Log
* Fixed issues with imagerySplitPosition and the international date line in 2D mode. [#5151](https://github.com/AnalyticalGraphicsInc/cesium/pull/5151)
* Fixed an issue with `TileBoundingBox` that caused the terrain to disappear in certain places [4032](https://github.com/AnalyticalGraphicsInc/cesium/issues/4032)
* `QuadtreePrimitive` now uses `frameState.afterRender` to fire `tileLoadProgressEvent` [#3450](https://github.com/AnalyticalGraphicsInc/cesium/issues/3450)
* Removed left, right, bottom and top properties from `OrthographicFrustum`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this up out of 1.32 changes.

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 11, 2017

Thanks for updating CHANGES.md, @ateshuseyin. Please comment on a PR after updating so the reviewers get notified.

@emackey please merge when ready.

@mramato
Copy link
Contributor

mramato commented Apr 17, 2017

@emackey is this good to go? If so, I can just reserve the conflict in CHANGES and merge. Thanks.

@emackey
Copy link
Contributor

emackey commented Apr 17, 2017

No, sorry, there are test failures here, on the removed properties.

bagnell
bagnell reviewed Apr 17, 2017
View reviewed changes
Source/Scene/OrthographicFrustum.js
@@ -86,15 +78,6 @@ define([
frustum._near = frustum.near;
frustum._far = frustum.far;

if (!frustum._useDeprecated) {
var ratio = 1.0 / frustum.aspectRatio;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to keep these statements because frustum._useDeprecated would always be false.

@ateshuseyin
Merge remote-tracking branch 'AnalyticalGraphicsInc/master' into remo…
6ca299b 
…ve-orthographicfrustum-properties

# Conflicts:
#	CHANGES.md
@ateshuseyin
Copy link
Contributor Author

@emackey there are test failures and I cannot find the right solution. Could you help me?

@emackey
Copy link
Contributor

emackey commented Apr 18, 2017

@ateshuseyin Oops, my previous comment was incorrect (so I deleted it).

Check out the code that @bagnell pointed out. You removed a whole if block, one that should evaluate to true now. If you restore the body of that block, the tests should pass.

@ateshuseyin
Copy link
Contributor Author

ateshuseyin commented Apr 24, 2017 •
edited
Loading

@emackey and @bagnell thank you for your help. I reverted if block w/o if(!this._useDeprecated). Tests now pass.

@emackey
Copy link
Contributor

emackey commented Apr 24, 2017

Thanks @ateshuseyin.

@bagnell tests pass now. Please merge when ready.

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 28, 2017

Thanks again @ateshuseyin!

@pjcozzi pjcozzi merged commit 852ba64 into CesiumGS:master Apr 28, 2017
@ateshuseyin ateshuseyin deleted the remove-orthographicfrustum-properties branch May 20, 2017 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bagnell bagnell bagnell left review comments

@emackey emackey emackey left review comments

Assignees
No one assigned
Labels
priority - next release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove deprecated OrthographicFrustum properties
6 participants
@ateshuseyin @hpinkos @bagnell @pjcozzi @mramato @emackey