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

3D-viewer v2 #1870

Merged
merged 16 commits into from Aug 22, 2018

Conversation

Projects
None yet
6 participants
@btzr-io
Copy link
Collaborator

btzr-io commented Aug 13, 2018

Todo [WIP]

  • Add controls. dat.gui
  • Minor Threejs optimizations
  • Fix memory leak #1863
  • Remove debug messages.
  • Minor geometry (BufferGeometry) optimization #1870 (review).
  • Fix material.flatShading.
  • Bug-fix: Set mesh.center as camera target for all loaders.
@kauffj

This comment has been minimized.

Copy link
Member

kauffj commented Aug 13, 2018

@seanyesmunt if you can tell @jamesbiller how to run this version he can confirm the issue has been fixed

@tzarebczan

This comment has been minimized.

Copy link
Member

tzarebczan commented Aug 13, 2018

@jamesbiller try this build here: http://build.lbry.io/app/build-3096_commit-612dd80/

I'll test it out today as well.

const geometry = new THREE.Geometry();
geometry.fromBufferGeometry(data);
geometry.computeBoundingBox();
geometry.computeVertexNormals();

This comment has been minimized.

@skhameneh

skhameneh Aug 14, 2018

Member

Might want to add geometry.mergeVertices();

and if you end up having issues mapping textures, this might help:

function updateGeometryUv(geometry) {
  let max = geometry.boundingBox.max,
      min = geometry.boundingBox.min;
  let offset = new THREE.Vector2(0 - min.x, 0 - min.y);
  let range = new THREE.Vector2(max.x - min.x, max.y - min.y);
  let faces = geometry.faces;

  geometry.faceVertexUvs[0] = [];

  for(let i = 0; i < faces.length ; i++) {

      let v1 = geometry.vertices[faces[i].a],
          v2 = geometry.vertices[faces[i].b],
          v3 = geometry.vertices[faces[i].c];

      geometry.faceVertexUvs[0].push([
          new THREE.Vector2((v1.x + offset.x)/range.x ,(v1.y + offset.y)/range.y),
          new THREE.Vector2((v2.x + offset.x)/range.x ,(v2.y + offset.y)/range.y),
          new THREE.Vector2((v3.x + offset.x)/range.x ,(v3.y + offset.y)/range.y)
      ]);
  }
  geometry.uvsNeedUpdate = true;
}

This comment has been minimized.

@skhameneh

skhameneh Aug 14, 2018

Member

Also, some of the operations might be faster if you can do them before you convert the GeometryBuffer into a Geometry

This comment has been minimized.

@btzr-io

btzr-io Aug 15, 2018

Author Collaborator

We don't really support textures ATM so that won't be a problem...

@skhameneh

This comment has been minimized.

Copy link
Member

skhameneh commented Aug 14, 2018

Looks good!

@tzarebczan

This comment has been minimized.

Copy link
Member

tzarebczan commented Aug 14, 2018

@btzr-io definitely noticing an improvement with memory usage after leaving the 3d model (takes a few seconds) - it does seem to release the memory allocated. If you open multiple after another, you can still get pretty high usage on some 3d models.

@jamesbiller how's this working on your PC?

@jamesbiller

This comment has been minimized.

Copy link

jamesbiller commented Aug 14, 2018

@tzarebczan It's been working a hell of a lot smoother! Good work

@btzr-io

This comment has been minimized.

Copy link
Collaborator Author

btzr-io commented Aug 19, 2018

Ready for review and more testing.

@seanyesmunt seanyesmunt assigned skhameneh and unassigned seanyesmunt Aug 20, 2018

@seanyesmunt seanyesmunt requested a review from skhameneh Aug 20, 2018

@seanyesmunt

This comment has been minimized.

Copy link
Member

seanyesmunt commented Aug 20, 2018

@btzr-io The new controls are awesome! It looks good from testing, I'll let @skhameneh do the dev review since he has a better understanding of the threejs stuff.

@tzarebczan

This comment has been minimized.

Copy link
Member

tzarebczan commented Aug 21, 2018

@btzr-io from initial testing, you could also check off Fix memory leak #1863 - it's definitely better in this version.

@seanyesmunt seanyesmunt force-pushed the three-v2 branch from 335d5bc to 710afc9 Aug 21, 2018

seanyesmunt added some commits Aug 21, 2018

@seanyesmunt seanyesmunt merged commit c077c91 into master Aug 22, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@seanyesmunt seanyesmunt deleted the three-v2 branch Sep 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.