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

Create a new preview pane that shows the Node tree also a GLB exporter. #35

Merged
merged 8 commits into from
Oct 28, 2017

Conversation

najadojo
Copy link
Contributor

@najadojo najadojo commented Oct 6, 2017

Also use npm dependencies to pull down Babylon.js instead of adding it to this project directly.

 Also use npm dependencies to pull down Babylon.js instead of adding it to this project directly.
@emackey
Copy link
Member

emackey commented Oct 7, 2017

Wow, cool, I wasn't expecting this! Before I dig into the code though, are you willing to send us a filled-out copy of the CLA (Contributor License Agreement)? This is a document that basically says that while you keep the copyright on the code you wrote, you did intend to share it with us and you grant us the rights to use, modify, and redistribute the code. You email it to cla@agi.com with a note that it's for gltf-vscode. Thanks!!

@najadojo najadojo changed the title Create a new preview pane that shows the Node tree. Create a new preview pane that shows the Node tree also a GLB exporter. Oct 10, 2017
@emackey
Copy link
Member

emackey commented Oct 10, 2017

Hi @najadojo, did you fill out a CLA? We can't take a pull request without this, unfortunately.

@najadojo
Copy link
Contributor Author

Working on the CLA... sorry for the delay.

@najadojo
Copy link
Contributor Author

najadojo commented Oct 24, 2017

I've just sent in my CLA. I'll merge master and do a new commit shortly.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 24, 2017

Thanks @najadojo, we received the CLA. It's awesome that you are contributing to this project in such a big way!

@emackey
Copy link
Member

emackey commented Oct 24, 2017

@HowardWolosky are you OK with switching Babylon to be an npm dependency?

@HowardWolosky
Copy link
Contributor

HowardWolosky commented Oct 24, 2017

I think it's a great idea. It certainly simplifies the update step for that engine. We should look at trying to do something similar for the other engines as well. It looks like we can do it for Three:

And I guess this would work for Cesium too?

Certainly doesn't need to be done as part of this PR, but we should open an item to do the work at some point. Any way we can remove manual effort would streamline things. This does put the onus on you though @emackey to be sure you're always running npm update before doing any release. You might want to add that to PUBLISH.md as a reminder.

Edit: This is now tracked by #41

@najadojo
Copy link
Contributor Author

Bit new at using GitHub... are there code review comments I'm missing? What would be the next steps to merge this PR? Thanks.

@emackey
Copy link
Member

emackey commented Oct 27, 2017

@najadojo Welcome! This is an amazing contribution, and I'm really excited about the GLB export in particular, it fills a great need in the glTF ecosystem that the glTF Working Group has been discussing recently.

One concern I have is that it looks like it would be easy for existing GLB files to be overwritten without warning. But the timing here is a happy coincidence, the VSCode team just last month added a shiny new file dialog API, including a window.showSaveDialog call that could be used to prompt for the GLB filename, defaulting to the name you've already calculated. I presume the file dialog would automatically show an overwrite warning, as most do, but I haven't tried this myself yet. A newer vscode API dependency is likely needed in package.json. Here's a link to where save dialogs were just recently added to the awesome vscode-hexdump extension.

Other than that I think this is good to go. Someday when I have more free time I may attempt to replace the HTML-based tree with VSCode's own tree, like the one used in vscode-zipexplorer. But that's a project for another time.

@HowardWolosky did you want to add any other comments here? I'll tweak the README myself, I think this just needs a save dialog and it's a go.

@najadojo
Copy link
Contributor Author

Oh cool I didn't know about that API. I was a bit unhappy with the filename selection and I did want to give the customer a way to make a selection so this is ideal. I also have plans to implement a GLB importer that would create a collection of separate files described within. Then a customer could easily open the resulting gltf text document and use the rest of the extension's features.

I specifically decided not to use the vscode tree view because I have further plans with that as well. I'd like to make the various buffers that make up meshes browseable in a sort of table view like you'd see in a Object Table. I'm not really sure how I'm going to accomplish as yet so suggestions would be helpful. Maybe using the tree view and show the buffers as documents as they are selected.

@emackey
Copy link
Member

emackey commented Oct 27, 2017

Sounds awesome! One other suggestion: When you start work on a new feature like GLB import, our usual best-practice is to create a new git branch named after the feature (such as glb-import or similar), and create the pull request from there. Typically features come in on separate branches so they can run at a different cadence (for example, a node-tree branch could get reviewed and merged separately from a glb-export branch, etc).

In this case it's not needed of course, just push the save dialog changes to your master branch when ready so they get added to this PR which is already based on master. My comments here are just for next time.

@HowardWolosky
Copy link
Contributor

I haven't had the time to really dig into this change. I think as long as @emackey has reviewed it and is ok with it, all seems good by me.

@emackey
Copy link
Member

emackey commented Oct 27, 2017

Cool. Got an error when saving to another folder, it tried to load the .bin file from the selected output folder instead of the gltf source folder.

@emackey
Copy link
Member

emackey commented Oct 28, 2017

Fixed a few issues and I'm merging this. Thanks so much @najadojo!

@emackey emackey merged commit 74a4eee into AnalyticalGraphicsInc:master Oct 28, 2017
@emackey
Copy link
Member

emackey commented Oct 28, 2017

And this is published now as 2.0.7. 🎉

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

4 participants