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

move scene logic to components (fixes #239, #759, #886) #776

Merged
merged 5 commits into from
Feb 1, 2016
Merged

move scene logic to components (fixes #239, #759, #886) #776

merged 5 commits into from
Feb 1, 2016

Conversation

ngokevin
Copy link
Member

Scene components:

  • canvas - Controls creation or selection of a canvas render target.
  • stats - Handles RStats.
  • keyboard-shortcuts
  • vr-mode-ui - Handles VR UI (button/modal).

Scene modules:

  • fullscreen - Handles fullscreen events.
  • iframe - all the postmesage stuff
  • meta-tags - Handles injecting of iOS metatags.
  • wakelock

Additional changes:

  • AEntity - HTMLElement.setAttribute to DOM for default components for scene.
  • canvas - Height/width configurable, triggers a render-target-loaded event on scene for scene to set up renderer.
  • look-controls - Point canvas to scene canvas.
  • stats - Put the UI within the scene.
  • Remove ?vr URL parameter, not sure if it is being used or actually needed in the core.
  • vr-mode-ui - Can be disabled. Put the UI within the scene.

Tested:

  • Some unit tests for new components
  • Macbook Pro on Firefox/Chrome. Enter/exit VR work for Firefox, modal is thrown on Chrome as before
  • On iPhone 6, enter/exit VR and orientation modal work as before

@ngokevin ngokevin mentioned this pull request Jan 13, 2016
Closed
@ngokevin ngokevin closed this Jan 13, 2016
@ngokevin ngokevin reopened this Jan 13, 2016
@ngokevin ngokevin closed this Jan 13, 2016
@ngokevin ngokevin reopened this Jan 13, 2016
caseyyee
caseyyee reviewed Jan 17, 2016
View reviewed changes
src/core/a-scene.js
@@ -93,24 +89,12 @@ var AScene = module.exports = registerElement('a-scene', {
var resizeCanvas = this.resizeCanvas.bind(this);
this.setupKeyboardShortcuts();
this.attachFullscreenListeners();
Copy link
Contributor

Choose a reason for hiding this comment

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

Fullscreen events should be ignored when vr-mode is forced to VR? Stereo rendering resets to mono when exiting fullscreen.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed my updated changes to this PR. Moved fullscreen stuff to https://github.com/aframevr/aframe/pull/776/files#diff-344cbe83c2f901ba50d1f8824d89ccfeR18

So you want the use case to always force stereo and completely ignore fullscreen events?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, debatable I suppose. my thought was that if you are going to manage your own viewport rendering, then you would want to also manually handle fullscreen events as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm a bit confused. Are you suggesting that if vr-mode is forced to VR, then ignore fullscreen events? And how should we allow vr-mode being forced to VR?

@ngokevin ngokevin force-pushed the scenecomponents branch 5 times, most recently from 667c838 to cf91fb8 Compare January 21, 2016 21:43
@cvan cvan modified the milestone: 0.2.0 Jan 21, 2016
@ngokevin
Copy link
Member Author

Okay after what must be a good week of working this, this is ready.

This was referenced Jan 22, 2016
Closed
Merged
dmarcos
dmarcos reviewed Jan 22, 2016
View reviewed changes
src/core/a-entity.js
rotation: '',
scale: '',
visible: ''
position: '0 0 0',
Copy link
Member

Choose a reason for hiding this comment

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

Can we set those to empty string so they're not defined for every entity?

@ngokevin ngokevin changed the title move scene logic to components move scene logic to components (fixes #759) Jan 22, 2016
@ngokevin ngokevin changed the title move scene logic to components (fixes #759) move scene logic to components (fixes #759, #886) Jan 22, 2016
@ngokevin ngokevin changed the title move scene logic to components (fixes #759, #886) move scene logic to components (fixes #239, #759, #886) Jan 22, 2016
@ngokevin
Copy link
Member Author

Updated. Moved fullscreen/metatags/wakelock to modules. Added keyboard-shortcuts scene component.

dmarcos
dmarcos reviewed Jan 27, 2016
View reviewed changes
src/core/a-entity.js
@@ -29,7 +29,7 @@ var AEntity;
* @member {boolean} paused - true if dynamic behavior of the entity is paused
*/
var proto = Object.create(ANode.prototype, {
defaults: {
defaultComponents: {
Copy link
Member

Choose a reason for hiding this comment

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

Are these changes related to the intent of the PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah wanted to expose the scene components in the DOM

dmarcos added a commit that referenced this pull request Feb 1, 2016
@dmarcos
Merge pull request #776 from ngokevin/scenecomponents
14e5503 
move scene logic to components (fixes #239, #759, #886)
@dmarcos dmarcos merged commit 14e5503 into aframevr:dev Feb 1, 2016
This was referenced Feb 1, 2016
Closed
Closed
@cvan cvan mentioned this pull request Feb 5, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
needs review
Projects
None yet
Milestone
0.2.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ngokevin @dmarcos @cvan @caseyyee @CodeRhymesLife