Update codebase to use Three.js version r88 #202

jihoonl · 2017-12-06T12:33:26Z

No description provided.

jihoonl · 2017-12-06T12:45:00Z

Well.. three@0.64.0 doesn't exist. The closest version is 0.66.0. Check if it is compatible.

…p.*.

jihoonl · 2017-12-08T13:36:01Z

Well.. let's bump the codebase to r88 instead of sticking with old version.

jihoonl · 2017-12-08T16:15:34Z

Hello @viktorku @sevenbitbyte , I have updated codebase to work with three.js r88. Please check if you have a time.

jihoonl · 2017-12-08T16:37:00Z

I have confirmed that marker, interactive marker, and urdf demos are working with this change.

T045T · 2017-12-11T13:23:46Z

build/ros3d.js

@@ -791,8 +790,8 @@ ROS3D.InteractiveMarker.prototype.onServerSetPose = function(event) {
      this.position.y = pose.position.y;
      this.position.z = pose.position.z;

-      this.quaternion = new THREE.Quaternion(pose.orientation.x, pose.orientation.y,
-          pose.orientation.z, pose.orientation.w);
+      this.quaternion.copy(new THREE.Quaternion(pose.orientation.x, pose.orientation.y,


you could probably use set() here, as you did with the other quaternions further down

T045T

Mostly cleanup, but you should fix highlighting :)

(And the one instance of quaternion.copy(new... where quaternion.set(... would have been nicer.)

T045T · 2017-12-11T13:30:30Z

src/visualization/interaction/Highlighter.js

-  scene.overrideMaterial = null;
+  else {
+    if(flag) {
+      object.currentMaterial = object.material;


shouldn't this be something like originalMaterial?

T045T · 2017-12-11T13:33:06Z

src/visualization/interaction/Highlighter.js

- * @param objects - the objects list to check
- * @param renderList - the list to add to
+ * @param object - the target object to (un)highlight
+ * @param flag - whether to highlight or unhighlight


these aren't terribly descriptive names - I'd prefer target_object and highlight instead

T045T · 2017-12-11T13:34:51Z

src/visualization/interaction/Highlighter.js

@@ -26,7 +26,8 @@ ROS3D.Highlighter = function(options) {
 * @param event - the event that contains the target of the mouseover
 */
 ROS3D.Highlighter.prototype.onMouseOver = function(event) {
-  this.hoverObjs.push(event.currentTarget);
+  // this.hoverObjs.push(event.currentTarget);


remove commented-out old code

T045T · 2017-12-11T13:34:57Z

src/visualization/interaction/Highlighter.js

@@ -35,62 +36,43 @@ ROS3D.Highlighter.prototype.onMouseOver = function(event) {
 * @param event - the event that contains the target of the mouseout
 */
 ROS3D.Highlighter.prototype.onMouseOut = function(event) {
-  this.hoverObjs.splice(this.hoverObjs.indexOf(event.currentTarget), 1);
+  // this.hoverObjs.splice(this.hoverObjs.indexOf(event.currentTarget), 1);


remove commented-out old code

T045T · 2017-12-11T13:36:39Z

src/interactivemarkers/InteractiveMarker.js

@@ -320,4 +321,5 @@ ROS3D.InteractiveMarker.prototype.dispose = function() {
  });
 };

-THREE.EventDispatcher.prototype.apply( ROS3D.InteractiveMarker.prototype );
+// THREE.EventDispatcher.prototype.apply( ROS3D.InteractiveMarker.prototype );


remove commented-out old code

T045T · 2017-12-11T13:39:05Z

src/models/Arrow.js

@@ -41,11 +41,12 @@ ROS3D.Arrow = function(options) {
  coneGeometry.applyMatrix(m);

  // put the arrow together
-  THREE.GeometryUtils.merge(geometry, coneGeometry);
+  // DEPRECATED: THREE.GeometryUtils.merge(geometry, coneGeometry);


remove commented-out old code

T045T · 2017-12-11T13:42:33Z

src/visualization/interaction/MouseHandler.js

@@ -69,7 +68,8 @@ ROS3D.MouseHandler.prototype.processDomEvent = function(domEvent) {
  var deviceX = left / target.clientWidth * 2 - 1;
  var deviceY = -top / target.clientHeight * 2 + 1;
  var vector = new THREE.Vector3(deviceX, deviceY, 0.5);
-  this.projector.unprojectVector(vector, this.camera);
+  // DEPRECATED: this.projector.unprojectVector(vector, this.camera);


remove commented-out old code

T045T · 2017-12-11T13:42:41Z

src/visualization/interaction/MouseHandler.js

@@ -221,4 +221,5 @@ ROS3D.MouseHandler.prototype.notify = function(target, type, event3D) {
  return 1; // Event Failed
 };

-THREE.EventDispatcher.prototype.apply( ROS3D.MouseHandler.prototype );
+// THREE.EventDispatcher.prototype.apply( ROS3D.MouseHandler.prototype );


remove commented-out old code

T045T · 2017-12-11T13:42:55Z

src/visualization/interaction/OrbitControls.js

@@ -501,4 +503,5 @@ ROS3D.OrbitControls.prototype.update = function() {
  }
 };

-THREE.EventDispatcher.prototype.apply( ROS3D.OrbitControls.prototype );
+// THREE.EventDispatcher.prototype.apply( ROS3D.OrbitControls.prototype );


remove commented-out old code

T045T · 2017-12-11T14:18:15Z

src/visualization/interaction/Highlighter.js

-          renderList.push(objlist[o]);
-          break;
-        }
+ROS3D.Highlighter.prototype.highlightObject = function (object, flag) {


This doesn't actually highlight correctly, but instead replaces the highlighted objects' materials with a half-transparent white material:

The old implementation rendered that material over the already-rendered scene to achieve the highlighted look.

Looks like current Three.js has a couple ways to make highlighting happen:
This demo (source) uses an OutlinePass to draw outlines, which looks really nice but is slightly more involved (here's a StackOverflow question with some very concise example code).

This one (source) simply gives the material a bit of (red) emissive lighting to achieve about the same effect we were using before (note they use currentHex to save the old value – I'd prefer to use originalHex).

The second example is the one I referenced to update the highlighting. Thanks for the review. I will take a look again.

jihoonl · 2017-12-11T14:43:06Z

Hello @DLu, @jstnhuang, and @rctoris,

I am trying to update the codebase to the latest three.js(r88) and found out that fetch_description isn't loadable due to this error in three.js. Could you help me to fix this issue? I have confirmed that I can visualize our robot, turtlebot2, and turtlebot3 with no problem.

T045T · 2017-12-12T14:13:04Z

I went ahead and sent some quick PRs to those repos - but of course, we should still be compatible with RViz' behavior.

For future reference, the command I used to automatically remove all up_axis tags was

find . -iname *.dae | xargs sed -i '/up_axis/d'

in the top-level workspace directory.

T045T · 2017-12-13T11:04:17Z

src/visualization/interaction/Highlighter.js

+      });
+    }
+    else {
+      target_object.material = target_object.originalMaterial;


One more thing: We should probably be defensive here and check for the existence of originalMaterial.

If highlightObject(false) is ever called on an object where highlight(true) has not been called, we'd be in trouble!

Yeah, I agree. I would improve this logic with another PR.

Please don't. Merging this PR as-is would introduce a serious bug (see the screen shot I attached to the issue), and fixing it would be trivial (just give the existing material an emissive component instead of switching the material completely).

It is not as easy as you described and it doesn't introduce serious bug while I was testing. This PR is already pretty large and getting longer and I am running out of time. Since it is already a big change which covers most of use cases. I will move on and start to patch small stuffs. I don't see any serious reason that this should not go into the develop branch.

…y it is forked

jihoonl · 2017-12-15T08:42:52Z

Summary

the forked collada loader is in examples/js/ColladaLoader2.js. It includes z_axis hack to avoid rviz issue that ignoring up_axis, and cherry-picked this commit
three.js version r88
Highlighter logic is fied to compatibly with r88. It still needs some more improvements. This will be fixed with the following PRs.
Note that browser doesn't support tif format. This needs to be converted to png format.

#!/bin/bash

for f in $(find -name *.tif); do
  echo "Converting $f"; convert "$f"  "$(dirname "$f")/$(basename "$f" .tif).png";
done

for f in $(find -name *.dae); do
  echo "Replacing $f"; sed -i -- 's/.tif/.png/g' $f
done

T045T · 2017-12-15T14:00:58Z

examples/js/ColladaLoader2.js

+ * Also it is renamed as ColladaLoader2.js
+ */
+
+THREE.ColladaLoader2 = function ( manager ) {


Why did you name it ColladaLoader2? It's ColladaLoader, and we actually want it to replace THREE.ColladaLoader completely, don't we?

On a higher level, do we even still need to support two loaders? Looks like what was once ColladaLoader2 has replaced the original ColladaLoader completely by now anyway.

Why do we need to replace it completely? It needs to be explicitly kept separate so that it indicates it is a modified version just for backward compatibility. Users should know that they use the modified version because their collada isn't following the standard.

I disagree.

My opinion is that users shouldn't have to care about what loader is used at all – de facto, we're not dealing with the Collada standard, but instead with the one set by RViz. From that perspective, all models that work in RViz are correct and we should display them the same way RViz does.

Either way, renaming the module to ColladaLoader2 is confusing because that's not what it is. It has the exact same interface as THREE.ColladaLoader.

Because of that, we could actually remove all the loader options from ros3djs and instead just use a different script tag to load either the official ColladaLoader version or this patched one:

  <script src="http://static.robotwebtools.org/fixes/ColladaLoader.js"></script>

Ok. reasonable. Also, I removed all loader option because it doesn't seem necessary.

T045T · 2017-12-15T14:02:36Z

examples/map.html

-<script src="http://cdn.robotwebtools.org/threejs/current/three.js"></script>
-<script src="http://cdn.robotwebtools.org/EventEmitter2/current/eventemitter2.min.js"></script>
-<script src="http://cdn.robotwebtools.org/roslibjs/current/roslib.js"></script>
+<script src="http://static.robotwebtools.org/threejs/current/three.js"></script>


these changes should really go in a separate PR - the CDN has already migrated, and we should reflect that in our code ASAP

It has been already requested to @rctoris. And it doesn't matter as long as it doesn't go in to master branch. It is a development branch. Of course, it will only go into master when it gets released after proper tests.

I'm not sure I understand what you're saying. The new URLs (with static in them) already work.

Why don't we make a separate pull request to update them in examples and Readme files right now, while we're still discussing the move to ThreeJS r88?

What I am saying is that our stable branch is master where we should keep the working code. I am moving on. We need to create a PR to fix some links example anyway when CDN is updated.

… exists in three.js

Small details will be addressed with latter PRs. This PR has reached enough level to be merged in to development branch for this such huge structural change.

jihoonl · 2017-12-15T16:22:39Z

@T045T sorry Nils. I had to proceed this PR because it was a definitely needed movement. Otherwise, this PR would hang for another year considering the recent year's activities in this repo.

RViz doesn't respect it anyway ( ros-visualization/rviz#1045 ), but current versions of three.js' ColladaLoader (used in the RobotWebTools, see RobotWebTools/ros3djs#202 ) do! (Since RViz doesn't respect the tag, this does not break the model)

jihoonl added 2 commits December 6, 2017 12:32

fix dependencies. and bump patch

8e09982

fix threejs

03bf58c

jihoonl changed the title ~~Ffix dependencies. and bump patch version~~ Fix dependencies. and bump patch version Dec 6, 2017

Jihoon Lee added 6 commits December 8, 2017 14:09

Use three@0.88.0

fde6a14

Use object assign instead of apply. See mrdoob/three.js#8775

e3cf789

Rename CubeGeometry to BoxGeometry. Changed r65->r66

ff8698d

Apply r66->r67

26f0cd8

r71->r72 Moved all shadowMap* properties in WebGLRenderer to shadowMa…

b261c28

…p.*.

Apply mrdoob/three.js#8775, and use unproject from vector

2c1270a

jihoonl changed the title ~~Fix dependencies. and bump patch version~~ Use Three.js version 0.88.0 Dec 8, 2017

jihoonl mentioned this pull request Dec 8, 2017

Update code base to work with modern three.js releases #158

Closed

jihoonl changed the title ~~Use Three.js version 0.88.0~~ Update codebase to use Three.js version 0.88.0 Dec 8, 2017

jihoonl changed the title ~~Update codebase to use Three.js version 0.88.0~~ Update codebase to use Three.js version r88 Dec 8, 2017

Jihoon Lee added 5 commits December 8, 2017 15:30

use project and unproject from vector object

7c78133

deal with immutable objects r67->r68

7a35e01

Update highlighting logic

58bb737

Update built modules

d35a129

Remove unused projector

5adaece

jihoonl requested review from sevenbitbyte and viktorku December 8, 2017 16:14

update built module

5b6c1d6

Jihoon Lee added 2 commits December 8, 2017 18:17

Fix color material. But please check if #187 break

49de6cf

update built module

c4d1b86

T045T reviewed Dec 11, 2017

View reviewed changes

T045T previously requested changes Dec 11, 2017

View reviewed changes

Jihoon Lee added 2 commits December 13, 2017 10:27

Renameing variables in Highlighter

44f5e34

update built module

400c2e8

T045T reviewed Dec 13, 2017

View reviewed changes

jihoonl changed the title ~~Update codebase to use Three.js version r88~~ Update codebase to use Three.js version r89 Dec 14, 2017

jihoonl changed the title ~~Update codebase to use Three.js version r89~~ WIP: Update codebase to use Three.js version r89 Dec 14, 2017

jihoonl changed the title ~~WIP: Update codebase to use Three.js version r89~~ Update codebase to use Three.js version r88 Dec 14, 2017

Jihoon Lee added 7 commits December 14, 2017 17:36

Add the forked verison of collada loader. See comments in the code wh…

c727ac9

…y it is forked

Assine material only if it is undefined

6d4e87f

fix cdn to static

7e0c1a7

set revision as develop

3d71d5d

let Meshresource to use the forked collada loader

1d082a4

update built module

49b82d3

Add fixit comment

587c9ca

update built module

3a48e09

T045T reviewed Dec 15, 2017

View reviewed changes

Jihoon Lee added 2 commits December 15, 2017 17:00

Completely remove ColladaLoader2 from ROS3D. ColladaLoader2 no longer…

0ca58f6

… exists in three.js

remove loader option

2f93d40

jihoonl merged commit c7a85b5 into develop Dec 15, 2017

jihoonl deleted the fix_dependencies branch December 15, 2017 16:15

T045T mentioned this pull request Dec 18, 2017

EventEmitter2 dependency not specified in package.json #194

Closed

DLu mentioned this pull request Feb 13, 2018

PointCloud2 broken in v0.18.0 #213

Closed

jihoonl mentioned this pull request Mar 6, 2018

Add ES6 module support #214

Merged

jihoonl mentioned this pull request Aug 7, 2020

Remove THREE.Group() and replace with THREE.Object3D(). #328

Merged

jihoonl mentioned this pull request Jul 5, 2021

Bump three from 0.89.0 to 0.128.0 #381

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update codebase to use Three.js version r88 #202

Update codebase to use Three.js version r88 #202

jihoonl commented Dec 6, 2017 •

edited

Loading

jihoonl commented Dec 6, 2017

jihoonl commented Dec 8, 2017

jihoonl commented Dec 8, 2017

jihoonl commented Dec 8, 2017

T045T Dec 11, 2017

T045T left a comment

T045T Dec 11, 2017

T045T Dec 11, 2017

T045T Dec 11, 2017

T045T Dec 11, 2017

T045T Dec 11, 2017

T045T Dec 11, 2017

T045T Dec 11, 2017

T045T Dec 11, 2017

T045T Dec 11, 2017

T045T Dec 11, 2017

jihoonl Dec 11, 2017

jihoonl commented Dec 11, 2017 •

edited

Loading

T045T commented Dec 12, 2017 •

edited

Loading

T045T Dec 13, 2017

jihoonl Dec 15, 2017

T045T Dec 15, 2017

jihoonl Dec 15, 2017

jihoonl commented Dec 15, 2017

T045T Dec 15, 2017

jihoonl Dec 15, 2017

T045T Dec 15, 2017

jihoonl Dec 15, 2017

T045T Dec 15, 2017

jihoonl Dec 15, 2017 •

edited

Loading

T045T Dec 15, 2017

jihoonl Dec 15, 2017

jihoonl commented Dec 15, 2017 •

edited

Loading

Update codebase to use Three.js version r88 #202

Update codebase to use Three.js version r88 #202

Conversation

jihoonl commented Dec 6, 2017 • edited Loading

jihoonl commented Dec 6, 2017

jihoonl commented Dec 8, 2017

jihoonl commented Dec 8, 2017

jihoonl commented Dec 8, 2017

Choose a reason for hiding this comment

T045T left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jihoonl commented Dec 11, 2017 • edited Loading

T045T commented Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jihoonl commented Dec 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jihoonl Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jihoonl commented Dec 15, 2017 • edited Loading

jihoonl commented Dec 6, 2017 •

edited

Loading

jihoonl commented Dec 11, 2017 •

edited

Loading

T045T commented Dec 12, 2017 •

edited

Loading

jihoonl Dec 15, 2017 •

edited

Loading

jihoonl commented Dec 15, 2017 •

edited

Loading