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

meshwalk.js updated three.js #3

Closed
wants to merge 5 commits into from
Closed

Conversation

nutiler
Copy link

@nutiler nutiler commented Sep 27, 2017

Hello!

This is such a great repository for game, unit collision, controls, and more! I started to work on a project using meshwalk.js and saw there were a few depreciated methods while updating from three.js r73+. I went ahead and updated it to the latest version of three.js which is r88 dev but I do not believe anything else will depreciate in the near future.

Some things such as Three.WireFrameHelper I commented out so all of the examples don't have any warnings or errors as I'll get to updating those using Three.WireFrameGeometry.

I updated the README for all of the changes.

Please review this pull and let me know if you have any questions!

Thanks,

Josh Wilson
josh@stormheart.net

@yomotsu
Copy link
Owner

yomotsu commented Sep 28, 2017

@nutiler Thanks for your contribution!

I wonder if you could load three.js from CDN. Could you change for all examples and delete three.js from this repo? Because it will cause large diff everytime.🙇
https://cdnjs.com/libraries/three.js/

Can we use es6-promise-auto instead of Bluebird.js? What we need is just loading following script instead of bluebard.js. Then, bluebird section in readme.md can be removed. (promise is already standard feature!)
https://cdnjs.cloudflare.com/ajax/libs/es6-promise/4.1.1/es6-promise.auto.min.js

I think we should have own EventDispacher in MeshWalk under MW namespace. I will prepare it later. In meantime, can we use THREE.EventDispatcher.prototype in three.js instead of external libraly?

README.md Outdated

`THREE.r88 dev Compatible! :)`

Copy link
Owner

Choose a reason for hiding this comment

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

I don't care older version compatibility for three.js. Could we remove this section?

Copy link
Author

Choose a reason for hiding this comment

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

Sure yeah, I was just writing that in case anyone has something in between 73-88 but those are obsolete with someone working on the latest version of threes and should update.

@@ -20,6 +20,8 @@
</div>

<script src="../lib/three.min.js"></script>
<script src="../lib/three.eventdispatcher.js"></script>

Copy link
Owner

Choose a reason for hiding this comment

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

could we just use THREE.EventDispatcher.prototype instead loading external lib?

Copy link
Author

Choose a reason for hiding this comment

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

https://github.com/mrdoob/three.js/blob/dev/build/three.module.js
https://github.com/mrdoob/eventdispatcher.js/blob/master/src/EventDispatcher.js

three.module.js has THREE.EventDispatcher with it, it was taken out of three.core.js when it was depreciated in 74 according to the documentation. It was then moved to its own repository/modules which is the only one I added, the CDN has the module available but I included it from the original repository.

@@ -70,7 +72,7 @@
);
ground.rotation.x = THREE.Math.degToRad( -90 );
scene.add( ground );
scene.add( new THREE.WireframeHelper( ground ) );
// scene.add( new THREE.WireframeHelper( ground ) );
Copy link
Owner

Choose a reason for hiding this comment

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

Could you uncomment for now or

var helper = new LineSegments(
  new WireframeGeometry( ground.geometry ),
  new LineBasicMaterial( { color: 0xffffff } )
);
scene.add( helper );

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I just haven't had time to go through and make the wireframes for the original objects!

@@ -6,7 +6,7 @@

ns.CharacterController = function ( object3d, radius ) {

THREE.EventDispatcher.prototype.apply( this );
Object.assign( ns.CharacterController.prototype, EventDispatcher.prototype );
Copy link
Owner

Choose a reason for hiding this comment

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

Can we just assign THREE. EventDispatcher .Prototype?

Copy link
Author

Choose a reason for hiding this comment

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

THREE.EventDispatcher.prototype was completely removed, in its place THREE.EventDispatcher.apply works in previous versions, but as of now, it states: EventDispatcher.apply has been removed. Inherit or Object.assign the prototype to mix-in instead.

@nutiler
Copy link
Author

nutiler commented Sep 29, 2017

@yomotsu

We can definitely switch to CDN's it will make it much smaller and cleaner as well, I'll be sure to do this next time I'm free. That's a great idea for promise-auto if we can make this more dependant on itself the better!

@@ -13,10 +13,14 @@
</div>

<script src="https://cdnjs.cloudflare.com/ajax/libs/three.js/87/three.min.js"></script>
<!--<script src="https://cdnjs.cloudflare.com/ajax/libs/three.js/87/three.module.js"></script>-->
Copy link
Owner

@yomotsu yomotsu Sep 30, 2017

Choose a reason for hiding this comment

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

three.module.js is for bundler and unnecessary for "static JS". Could you remove this line please?

</style>
<meta charset="utf-8">
<title>1: Getting Started</title>
<link rel="stylesheet" href="./css/examples.css">
Copy link
Owner

@yomotsu yomotsu Sep 30, 2017

Choose a reason for hiding this comment

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

I was wondering if you could focus on main subject and purpose which support latest three.js, in this PR.

Otherwise, I cannot approve to merge until all change will be appropriated. I don't want to separate CSS from HTML source, as an example source, as well as some indent and spacing rules.🙇

You can separate this kind of extra change to another PR later.

I'm really sorry to have asked you so much. But I just want you to join as the first contributor🙇

But if you feel like it's a burden on you, feel free to leave it to me 👍

Copy link
Author

Choose a reason for hiding this comment

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

Heya I've been busy I was wondering if you've had a chance to look at the last commit?

Copy link
Owner

Choose a reason for hiding this comment

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

sorry for very late...
seems your commits still contains unnecessary changes 😢
Could you check the diff please?

top: 0;
left: 0;
}
</style>
Copy link
Owner

Choose a reason for hiding this comment

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

could you revert indents?


})();
Copy link
Owner

@yomotsu yomotsu Oct 22, 2017

Choose a reason for hiding this comment

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

some spacings have been also changed
I prefer to keep kind of mrdoob's code style

@Thundros
Copy link

Thundros commented Apr 22, 2018

@yomotsu Please update the examples to use r92. ESPECIALLY the mesh_walk one & mobuko walkthrough one! Thank you!

@yomotsu
Copy link
Owner

yomotsu commented Apr 24, 2018

@Thundros sorry, I forgot this. I will update sometime soon!

@Thundros
Copy link

@yomotsu how long? that's ok :)

@Thundros
Copy link

?

@yomotsu
Copy link
Owner

yomotsu commented Apr 26, 2018

maybe this weekend

@Thundros
Copy link

Really?! That would be lovely! THANK you, @yomotsu !

@Thundros
Copy link

Hi @yomotsu ! Excellent as always on the update! Just thought I might mention that if you scale the terrain to say 20, 20, 20, it will make Miku fall through the terrain if the Terrain gets steep. How would I fix this? Thank you!

@Thundros
Copy link

Thanks alot, @yomotsu !

@yomotsu yomotsu closed this Apr 29, 2018
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