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 an animation value editor #112

Merged
merged 3 commits into from
May 24, 2018

Conversation

najadojo
Copy link
Contributor

No description provided.

@emackey
Copy link
Member

emackey commented Apr 25, 2018

Thanks @najadojo, this looks very cool. I'll give it a try soon.

Copy link
Member

@emackey emackey left a comment

Choose a reason for hiding this comment

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

Overall this is really cool. It's quite the power-user feature, so I'm not sure if we should be doing more to protect new users from damaging their files with this.

Anyway I plan to merge this. I did leave a few random questions below, feel free to say no to any of them.

src/extension.ts Outdated
fs.writeFileSync(targetFilename, finalBuffer, 'binary');

bufferJson.byteLength = finalBuffer.length;
const newJson = JSON.stringify(glTF, null, ' ');
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can respect the user's editor.tabSize and editor.insertSpaces settings here? Something like this, converted from JS to TS...

var separator = useSpaces ? (new Array(tabSize + 1)).join(' ') : '\t';

src/extension.ts Outdated
editBuilder.replace(new vscode.Range(pointer.value.line, pointer.value.column,
pointer.valueEnd.line, pointer.valueEnd.column), newJson);
});
await vscode.commands.executeCommand('editor.action.formatDocument', activeTextEditor.document.uri);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to reformat the whole doc here? Granted it will reformat on export, so I guess it's OK...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I'm changing this to use the indent settings as suggested above and indenting the edited selection 4 times.

@@ -89,6 +89,37 @@ This works for arrays as well, for example the list of enabled render states. H

![Render states enable](images/StatesEnable.png)

### • Animation editor

Press <kbd>ALT</kbd> + <kbd>i</kbd> on an animation sampler to import values into the glTF JSON.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a warning here that this will reformat the whole document?

src/extension.ts Outdated
bufferJson.byteLength = finalBuffer.length;
const newJson = JSON.stringify(glTF, null, ' ');
await activeTextEditor.document.save();
fs.writeFileSync(activeTextEditor.document.fileName, newJson, 'utf8');
Copy link
Member

Choose a reason for hiding this comment

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

Minor wrinkle, but if the 3D preview window is open when this happens, it doesn't appear to automatically update with the new changes. I only tested once though, but I pressed Alt-o and it saved the file, and the 3D window didn't blink or reload. I had to manually make another harmless change to the spacing and save that to force a reload.

Copy link
Member

Choose a reason for hiding this comment

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

Messing with this some more. If the document is un-saved with edits and you press alt-o, the act of saving causes a 3D preview reload, but it reloads the wrong version (the version being saved, with the edits still in the extras, not the exported version). If the user does manually save some edits, the reload will be triggered then, and no reload will happen on alt-o. Apparently neither case results in the preview window showing the newly written bin file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latest changes don't save and just do in place editing. I've added a note in the readme suggesting to the customer to save when they are ready.

@najadojo
Copy link
Contributor Author

najadojo commented May 4, 2018

I haven't forgot about this feedback, I've just been very busy this week. Hopefully soon I'll get a chance to revisit this.

README.md Outdated
},
```

Modify the `vscode_gltf_input` and `vscode_gltf_output` arrays for your needs and then press <kbd>ALT</kbd> + <kbd>o</kbd> to rewrite the glTF buffer and JSON with new values. **Warning** this will overwrite the existing binary buffer and can not be undone. Save the edited document and use `glTF: Preview 3D Model` to view your changes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other thought I had here is create a new file name for the buffer each time. This would allow undo/redo to function as expected but would dump a lot of garbage files in the asset path. I looked for undo/redo overrides and didn't see much; maybe I'll open a feature request so we can rerun the edits that were made.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. I do like the idea of leaving the original buffer intact, and adding new buffers for hand-edited data. In fact among my own files I've got a few glTFs where I've hand-edited the JSON and saved it with a modified name, still containing a reference to the original bin file. That sort of thing results in a folder with a single bin file shared by several similarly-named glTFs with different settings/materials/node transforms/etc. In my case it's mostly for debug/test, but I wonder if users also have multiple glTFs in one folder referencing a shared bin file.

Also, we could advise users that if they end up with some of these "separate" animation bin files, they can export to GLB to bring all the binary data together. In fact they can even re-import from the exported GLB to get a new single bin file with their hand-edits embedded.

src/extension.ts Outdated

bufferJson.uri = 'data:application/octet-stream;base64,' + finalBuffer.toString('base64');
Copy link
Member

Choose a reason for hiding this comment

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

This is genius. 👍

Copy link
Member

@emackey emackey left a comment

Choose a reason for hiding this comment

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

Awesome.

@emackey emackey merged commit ad379f0 into AnalyticalGraphicsInc:master May 24, 2018
@emackey
Copy link
Member

emackey commented Jun 1, 2018

This is published now with version 2.1.13. Thanks again @najadojo.

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

2 participants