-
Notifications
You must be signed in to change notification settings - Fork 303
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
chore: update packages #1977
chore: update packages #1977
Conversation
@mgermerie beware of Could you check the change logs of dependencies to apply new features in itowns? |
Yep, I'll check that and remove node-fetch update 👍 |
180f745
to
1ec89f2
Compare
760811f
to
243d618
Compare
acc20d6
to
6a3834d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only two minor comments, otherwise it's good for me. Should we open an issue for the two remaining packages?
I have tested the LegacyGLTFLoader with a 3D Tiles that has a gltf in version 1.0 and it works.
examples/js/ThreeLoader.js
Outdated
@@ -41,7 +41,7 @@ ThreeLoader.getThreeJsLoader = function getThreeJsLoader(format) { | |||
const deferredPromise = defer(); | |||
// eslint-disable-next-line no-undef | |||
THREE = itowns.THREE; | |||
loadScriptAsync('https://cdn.rawgit.com/mrdoob/three.js/r' + itowns.THREE.REVISION + '/examples/js/loaders/' + format + 'Loader.js') | |||
loadScriptAsync('https://cdn.rawgit.com/mrdoob/three.js/r147/examples/js/loaders/' + format + 'Loader.js') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment at the beginning of the file explaining that it gets loaders of three version 147 please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you remove the dynamic loading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because since r147, the examples/js
folder is no longer. loaders are now in the form of js modules which can't be loaded that way. I tried to fix this in the Collada example (since it is the only one to use ThreeLoader
) but we would need to be able to use import statements inside the examples, which did not work for some reason. I will open an issue on this specific point but as it is only for Collada, I figured we could have this temporary fix just to allow updating three for the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you use examples/jsm
instead of examples/js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Collada loader in jsm uses import statements which cannot be used outside modules. ThreeLoader does not load loaders as modules. ThreeLoader needs to be removed in my opinion, since it is only used to load collada loader in a way that is no longer supported by three (loading js scripts directly). Instead, we could refactor all the examples to use import statement (like what is done here for instance). It would be closer to what users do, since importing iTowns and every other dependency with a <script>
tag is not verry npm and webpack friendly. This would however take a bit of time to work on and would further delay threejs update, hence the temporary solution I suggest here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this example, bellow, works but it needs to add three
when the site is deployed
<script type="importmap">
{
"imports": {
"three": "../node_modules/three/build/three.module.js",
"three/addons/": "../node_modules/three/examples/jsm/"
}
}
</script>
<script type="module">
import { ColladaLoader } from 'three/addons/loaders/ColladaLoader.js';
// ---------- CREATE A GlobeView FOR SUPPORTING DATA VISUALIZATION : ----------
// Define camera initial position
const placement = {
coord: new itowns.Coordinates('EPSG:4326', 4.21655, 44.84415),
range: 500,
heading: 180,
tilt: 60,
};
// `viewerDiv` will contain iTowns' rendering area (`<canvas>`)
const viewerDiv = document.getElementById('viewerDiv');
// Create a GlobeView
const view = new itowns.GlobeView(viewerDiv, placement);
// Setup loading screen and debug menu
setupLoadingScreen(viewerDiv, view);
const debugMenu = new GuiTools('menuDiv', view);
// ---------- DISPLAY CONTEXTUAL DATA : ----------
// Display ortho-images
itowns.Fetcher.json('./layers/JSONLayers/Ortho.json')
.then(function _(config) {
config.source = new itowns.WMTSSource(config.source);
view.addLayer(
new itowns.ColorLayer('Ortho', config),
).then(debugMenu.addLayerGUI.bind(debugMenu));
});
// Display elevation data
function addElevationLayerFromConfig(config) {
config.source = new itowns.WMTSSource(config.source);
view.addLayer(
new itowns.ElevationLayer(config.id, config),
).then(debugMenu.addLayerGUI.bind(debugMenu));
}
itowns.Fetcher.json('./layers/JSONLayers/IGN_MNT_HIGHRES.json')
.then(addElevationLayerFromConfig);
itowns.Fetcher.json('./layers/JSONLayers/WORLD_DTM.json')
.then(addElevationLayerFromConfig);
const loadingManager = new itowns.THREE.LoadingManager();
const colladaLoader = new ColladaLoader(loadingManager);
const url = 'https://raw.githubusercontent.com/iTowns/iTowns2-sample-data/master/models/collada/building.dae';
const fn = (collada) => {
const model = collada.scene;
// building coordinate
const coord = new itowns.Coordinates(
'EPSG:4326', 4.2165, 44.844, 1417,
);
model.position.copy(coord.as(view.referenceCrs));
// align up vector with geodesic normal
model.lookAt(model.position.clone().add(coord.geodesicNormal));
// user rotate building to align with ortho image
model.rotateZ(-Math.PI * 0.2);
model.scale.set(1.2, 1.2, 1.2);
// update coordinate of the mesh
model.updateMatrixWorld();
view.scene.add(model);
view.notifyChange();
};
colladaLoader.load(url, fn);
</script>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the example suggestion. I used it in #2151. As the static import was merged in #2127 and is thus in the master branch, I think we can merge this PR to benefit the packages updates and fix the static import in another PR. WDYT @jailln and @gchoqueux ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok for me
examples/js/ThreeLoader.js
Outdated
@@ -41,7 +41,7 @@ ThreeLoader.getThreeJsLoader = function getThreeJsLoader(format) { | |||
const deferredPromise = defer(); | |||
// eslint-disable-next-line no-undef | |||
THREE = itowns.THREE; | |||
loadScriptAsync('https://cdn.rawgit.com/mrdoob/three.js/r' + itowns.THREE.REVISION + '/examples/js/loaders/' + format + 'Loader.js') | |||
loadScriptAsync('https://cdn.rawgit.com/mrdoob/three.js/r147/examples/js/loaders/' + format + 'Loader.js') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you remove the dynamic loading?
@mgermerie Coud you update #1725 with the new issues? |
could add There's fetching issue |
Do you mean like that : 41be30d ? |
yes |
41be30d
to
a5840ce
Compare
could you update THREE to r154 |
a5840ce
to
8b939f6
Compare
done |
3a9ff9a
to
3cf0298
Compare
ab10ff8
to
9476db9
Compare
d414dba
to
e611ec2
Compare
e611ec2
to
a50292d
Compare
Description
Update the following packages (the checked packages' changelog was watched and eventual new features / breaking changes were applied) :
Updates skipped for now :
TODO :
GLTFLoader
has to be updated because of mrdoob/three.js@773bb2e in thee r153. → done in 4fad733THREE.RGBFormat
has been deprecated since r137. It was removed in r153 (in mrdoob/three.js@88e8954). It has to be updated inGLTFLoader
and inLegacyGLTFLoader
(here). → done in 4fad733URL
class, not the mocked one in bootstrap.js → done in a632611webpack.config
.ColladaLoader
only exists in .jsm since r148. → Temporary fix done. Need to find a more suitable solution in the future20.5.0
fails. I shall investigate this. → Will do it in another PR.Note :
Changes on threejs renderer output
ColorSpace
in r152 make some objects appear brighter.