-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fix #12532: Defers batch destruction until next update #12824
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for the pull request, @Jean-Dum! ✅ We can confirm we have a CLA on file for you. |
b19a430
to
450e2e1
Compare
Thanks for the PR @Jean-Dum! @mzschwartz5 could you please review? |
Sorry for the delay- this slipped off my list but I just remembered it and will be reviewing shortly. |
package.json
Outdated
"eslint-config-prettier": "^10.1.8", | ||
"eslint-plugin-html": "^8.1.1", | ||
"eslint-plugin-n": "^17.21.3", |
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.
Not sure why these changes are committed; I don't think any packages should need updating for this PR.
(I assume this was in an attempt to resolve some build errors you were having; shouldn't need this though)
Hi @Jean-Dum, I pulled down your changes (minus the Otherwise, the changes look good to me. Obviously, I'd want you to test them first, and we'll also need the |
450e2e1
to
2af30b0
Compare
2af30b0
to
e04df47
Compare
@mzschwartz5 Thanks for your answer! I reverted my [13:04:36] 'build' errored after 27 s
[13:04:36] Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'yaml' imported from C:\Users\jedum\Documents\cesium\packages\sandcastle\scripts\buildGallery.js
at Object.getPackageJSONURL (node:internal/modules/package_json_reader:316:9)
at packageResolve (node:internal/modules/esm/resolve:768:81)
at moduleResolve (node:internal/modules/esm/resolve:858:18)
at defaultResolve (node:internal/modules/esm/resolve:990:11)
at #cachedDefaultResolve (node:internal/modules/esm/loader:755:20)
at ModuleLoader.resolve (node:internal/modules/esm/loader:732:38)
at ModuleLoader.getModuleJobForImport (node:internal/modules/esm/loader:317:38)
at #link (node:internal/modules/esm/module_job:208:49)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] build: `gulp build`
npm ERR! Exit status 1 Or sometimes this issue (don't know why it isn't the same issue each time): [14:42:26] TypeError [ERR_INVALID_MODULE_SPECIFIER]: Invalid module ".\file:\C:\Users\jedum\Documents\cesium\packages\sandcastle\sandcastle.config.js" is not a valid package name imported from C:\Users\jedum\Documents\cesium\scripts\build.js
at parsePackageName (node:internal/modules/package_json_reader:272:11)
at Object.getPackageJSONURL (node:internal/modules/package_json_reader:283:53)
at packageResolve (node:internal/modules/esm/resolve:768:81)
at moduleResolve (node:internal/modules/esm/resolve:858:18)
at defaultResolve (node:internal/modules/esm/resolve:990:11)
at #cachedDefaultResolve (node:internal/modules/esm/loader:755:20)
at ModuleLoader.resolve (node:internal/modules/esm/loader:732:38)
at ModuleLoader.getModuleJobForImport (node:internal/modules/esm/loader:317:38)
at onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:683:36)
at TracingChannel.tracePromise (node:diagnostics_channel:350:14)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] build: `gulp build`
npm ERR! Exit status 1 I think the dependencies in Sorry for wasting your time for solving those little issues, I will make the better pull request possible after that 😊 |
Hi @Jean-Dum, The zip issue your encountering was fixed recently. I suspect your local branch isn't up to date with main. If you merge int he latest changes, that issue should go away. Your other issues probably stem from not having a clean workspace before npm installing. I would try the following: The choice to have |
Description
This follows the discussion in the issue #12532.
The changes made in #12722 where only made on the
StaticGroundGeometryPerMaterialBatch.js
file, but should have been made on the otherStatic...Batch
files (as the other material types continue to blink when concerned materials are updated).I ported the changes of this PR on three other
Static...Batch
files. (the two others does not seems to need those changes).Issue number and link
Testing plan
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change