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

Inline and simplify sketch.js in DCGAN example #1074

Merged
merged 4 commits into from
Nov 4, 2020

Conversation

josephrocca
Copy link
Contributor

@josephrocca josephrocca commented Oct 9, 2020

β†’ Step 1: Which branch are you submitting to? 🌲

Development

β†’ Step 2: Describe your Pull Request πŸ“

This just simplifies the examples/javascript/DCGAN/DCGAN_Random example by inlining the ~50 line sketch.js and reducing it down to about 10 lines.

β†’ Step 3: Share a Relevant Example πŸ¦„

No codepen/fiddle since I don't think I'd be able to host the models shards anywhere?
Just realised that the face model somehow loads from github (thought there'd be CORs issues): https://jsbin.com/posihevawe/2/edit?html,output

β†’ Step 4: Screenshots or Relevant Documentation πŸ–Ό

It works just the same as before:

image

Thanks! 🌈
Joe

`sketch.js` is ~50 lines, but it can be simplified to about 9 lines
@josephrocca josephrocca changed the title Patch 1 Inline and simplify sketch.js in DCGAN example Oct 9, 2020
Copy link
Member

@bomanimc bomanimc left a comment

Choose a reason for hiding this comment

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

This change makes sense to me! I think it makes the core functionality in the example easier to understand since there's less code associated with creating the canvas elements and buttons. That said, most of our examples do not use inline JS code, so we might be breaking consistency. Overall, I still feel that this would be a good change to merge in.

What do you think @shiffman @joeyklee?

examples/javascript/DCGAN/DCGAN_Random/index.html Outdated Show resolved Hide resolved
@shiffman shiffman self-requested a review October 15, 2020 02:43
Copy link
Member

@shiffman shiffman left a comment

Choose a reason for hiding this comment

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

I think this looks great and much more inline with JS conventions!

@bomanimc bomanimc changed the base branch from development to main October 29, 2020 03:21
@bomanimc bomanimc merged commit f3c8dfd into ml5js:main Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants