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

B3dm external textures #4376

Merged
merged 5 commits into from
Oct 3, 2016
Merged

B3dm external textures #4376

merged 5 commits into from
Oct 3, 2016

Conversation

sumitshyamsukha
Copy link
Contributor

Fixes #3892

@sumitshyamsukha sumitshyamsukha deleted the b3dm-external-textures branch September 28, 2016 14:01
@sumitshyamsukha
Copy link
Contributor Author

@lilleyse I'm not sure why this PR ended up including all of the previous commits -- I tried cherry-picking, but that didn't work either. Any suggestions on how to submit only the one commit?

@lilleyse
Copy link
Contributor

Make sure the base branch that you are merging into is AnalyticalGraphicsInc:3d-tiles. You can reopen this PR and change the target branch by clicking the edit button at the top.

@sumitshyamsukha sumitshyamsukha restored the b3dm-external-textures branch September 28, 2016 20:29
@sumitshyamsukha sumitshyamsukha changed the base branch from master to 3d-tiles September 28, 2016 20:30
@sumitshyamsukha
Copy link
Contributor Author

@lilleyse Thanks so much! I tested this and it seems to fix the issue. Please let me know if there's anything else that needs to be fixed in this PR.

@lilleyse
Copy link
Contributor

@sumitshyamsukha Can you change this PR so that it only includes the external textures commit, and open another PR for just the shadows changes.

After that, you will have to resolve the merge conflict. Usually the best approach is to switch to the 3d-tiles branch, git pull origin 3d-tiles (make sure that origin references the AnaylticalGraphics origin with git remote -v), then checkout the b3dm-external-textures branch, and do git merge 3d-tiles. Edit the code to fix any merge conflicts, add the fixed files, then git commit.

@lilleyse lilleyse mentioned this pull request Sep 29, 2016
Merged
@lilleyse
Copy link
Contributor

Is this ready? After updating a PR just leave a small comment to let me know to check it out since I usually ignore emails about pushed commits.

@sumitshyamsukha
Copy link
Contributor Author

sumitshyamsukha commented Sep 29, 2016 •
edited
Loading

Sorry for not leaving a comment! This PR is ready! I'm still trying to fix the merge conflict on the other PR.

lilleyse
lilleyse reviewed Sep 29, 2016
View reviewed changes
Source/Scene/Batched3DModel3DTileContent.js
@@ -256,7 +258,7 @@ define([
gltf : gltfView,
cull : false, // The model is already culled by the 3D tiles
releaseGltfJson : true, // Models are unique and will not benefit from caching so save memory
basePath : this._url,
basePath : getBaseUri(this._url, false),
Copy link
Contributor

Choose a reason for hiding this comment

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

Leave out the false, by default it is true which is good since we want to keep the query string.

lilleyse
lilleyse reviewed Sep 29, 2016
View reviewed changes
Source/Scene/Instanced3DModel3DTileContent.js
@@ -314,7 +316,7 @@ define([
collectionOptions.url = joinUrls(baseUrl, gltfUrl);
} else {
collectionOptions.gltf = gltfView;
collectionOptions.basePath = this._url;
collectionOptions.basePath = getBaseUri(this._url, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here.

@lilleyse
Copy link
Contributor

lilleyse commented Oct 3, 2016

I think something went wrong with the merge. The files changed section includes a lot of extraneous files. I would just delete that commit and try the merge again.

This sequence of commands works for me:

git checkout 3d-tiles
git pull origin 3d-tiles
git checkout b3dm-external-textures
git merge 3d-tiles

@sumitshyamsukha
Copy link
Contributor Author

@lilleyse Updated, should be fine now!

@lilleyse
Copy link
Contributor

lilleyse commented Oct 3, 2016

Thanks @sumitshyamsukha it looks great now!

@lilleyse lilleyse merged commit 81edb91 into CesiumGS:3d-tiles Oct 3, 2016
@pjcozzi pjcozzi mentioned this pull request Oct 10, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lilleyse lilleyse lilleyse left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sumitshyamsukha @lilleyse