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

Issue/15 implement 3 d model page route #27

Open
wants to merge 184 commits into
base: master
Choose a base branch
from

Conversation

emanuelGitCodes
Copy link

Review changes done to implement 3d models.

Former-commit-id: 0b18ad9
@clpetersonucf
Copy link
Member

This is continuing to look better and better. Some more feedback for the creator specifically, as the experience becomes more refined:

  • The media upload dialog no longer displays after you close the intro dialog. I have to select "Choose a different image" manually.
  • The "Rotating Model" button is a little deceptive, since that's the current state but gives no hint as to what clicking the button should actually do. I would prefer it accurately describe the action, something like "Toggle label placement" and "Toggle rotation"
  • There really needs to be a keyboard shortcut to enable rotation mode (while the key is held) when in label placement mode
  • The "Rotating"/"Labeling" indicator on the bottom-left can be clicked, and doing so causes the indicator to expand and fill the entire left-hand menu bar. I'm not sure if that's intentional.
  • "Scale Up"/"Scale Down" should be more descriptive. Maybe a sub-header above it to indicate what you're scaling: "Label anchor size" or something.
  • Anchor scaling is a little inconsistent. In the model I tried, I scaled down the anchors by a size or two, then saved and reloaded the creator - the anchors were a different size altogether. Additionally, the size of the anchors does not appear to be saved to the qset, or is not applied correctly when the qset is reloaded.
  • Hovering a label correctly recolors the anchors, but saving and reloading causes a console error when I hover over an anchor, and the anchor does not change colors.
  • Anchors should probably have a minimum and maximum size they can be scaled to (so they don't disappear completely)

@emanuelGitCodes emanuelGitCodes force-pushed the issue/15-Implement-3D-model-PageRoute branch from d32089a to 9c878f3 Compare December 15, 2021 16:33
…anuelGitCodes/labeling-materia-widget into issue/15-Implement-3D-model-PageRoute
Copy link
Contributor

@FrenjaminBanklin FrenjaminBanklin left a comment

Choose a reason for hiding this comment

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

It looks like the CoffeeScript files are no longer necessary, according to the Webpack configuration.

Remove any files that are not necessary and do a sweep for leftover console.logs.

src/core3D.js Outdated
intersects[0].uv
)
}
console.log(intersects[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't leave in this console.log.

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member

@clpetersonucf clpetersonucf left a comment

Choose a reason for hiding this comment

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

In addition to my inline comments, the coffeeBag and models3D directories should be removed, except for anything required by the demo.

@@ -14,11 +14,13 @@ Namespace('Labeling').Creator = do ->
# store image dimensions in case the user cancels the resize
_lastImgDimensions = {}

# track if the user is "getting started" or well on their way
# track if the user is "get`ting started" or well on their way
Copy link
Member

Choose a reason for hiding this comment

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

Typo?

@@ -1,8 +1,12 @@

# Does not import
# import './build/three.js'
Copy link
Member

Choose a reason for hiding this comment

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

If this is leftover or placeholder code, it should probably be removed.

src/core3D.js Outdated

function onError(error) {
// Returns a console log ERROR when model doesn't load
console.log('ERROR: ' + error)
Copy link
Member

Choose a reason for hiding this comment

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

I would use console.warn or console.error for this one.

src/core3D.js Outdated
intersects[0].uv
)
}
console.log(intersects[0])
Copy link
Member

Choose a reason for hiding this comment

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

+1

src/creator.js Outdated
// Temporarily disable while using MWDK

// Using this style of import will cause the 3D to run every time even when on 2D.
// import { renderedSpheresGroup } from './core3D.js'
Copy link
Member

Choose a reason for hiding this comment

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

If this is placeholder code, it should be removed

src/creator.js Outdated

function qsetOption3D(_qset) {
let _anchorOpacityValue = 1.0
console.log(highlightSpheresGroup.children[0])
Copy link
Member

Choose a reason for hiding this comment

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

Should remove unnecessary console logs

src/creator.js Outdated
sphereScale: highlightSpheresGroup.children[0].scale
}

console.log(_qset.options.sphereRadius)
Copy link
Member

Choose a reason for hiding this comment

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

Remove unnecessary console logs

src/creator.js Outdated
let keyboard = document.createElement('div')
keyboard.id = 'keyboard-text'
keyboard.classList.add('keyboardText')
keyboard.innerHTML = 'Arrow Keys:'
Copy link
Member

Choose a reason for hiding this comment

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

We can condense this string to a single line, instead of concatenating multiple strings and using the <br /> tag:

keyboard.innerHTML = 'Arrow Keys: \n &#183; Rotate camera. \n Option/alt: \n &#183; While pressing change between rotating and labeling'

You'll also have to add a new style in your CSS:

.keyboardText {
    white-space: pre;
}

function mouseText() {
let mouse = document.createElement('div')
mouse.id = 'mouse-text'
mouse.classList.add('keyboardText')
Copy link
Member

Choose a reason for hiding this comment

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

Same deal as my comment on line 1314

src/creator.js Outdated
label.setAttribute('data-y', vector.y)
})

return _drawBoard()
Copy link
Member

Choose a reason for hiding this comment

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

Return not required

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.

3 participants