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

Replace dat.gui with lil-gui #22765

Merged
merged 4 commits into from
Nov 3, 2021
Merged

Replace dat.gui with lil-gui #22765

merged 4 commits into from
Nov 3, 2021

Conversation

georgealways
Copy link
Contributor

@georgealways georgealways commented Oct 31, 2021

So there were 120 examples using dat.gui, and 3 more pages in docs/scenes. After swapping the import URL for lil-gui, I went through and moved a few controllers on each page. All told, there were 5 that broke and needed more attention.

Broken ones

docs/scenes/bones-browser

  • Referenced the .__controllers array which is just called .controllers now.

examples/webgl_instancing_performance

  • Appended some debug text to dat.gui's .__ul element which is now .$children

examples/webgl_tonemapping

  • Called gui.remove( controller ) which is now controller.destroy()

examples/webgl_animation_skinning_additive_blending

  • Traversed dat.gui's DOM to apply an active/inactive style.

examples/webgl_animation_skinning_blending

  • Interacted w/ dat.gui's DOM to disable/enable buttons. That's built in now.

Side note: That last example was updating the disable/enabled state of its buttons by disabling them all, testing the animation state, and then re-enabling the appropriate buttons every animation frame. lil-gui will ignore redundant calls to disable/enable, but it can't tell in this case, since every controller is always disabled at the beginning of the loop. I've changed that code so it's not triggering DOM updates every frame.

Cosmetic fixes

Thankfully nothing else I found needed fixes to run. The rest was "while I'm at it" type of stuff, but nothing heavy handed.

These examples set a larger width than necessary, so I either made the width smaller or lost the width parameter entirely. lil-gui wins back some pixels by dropping the left-side color strips and the indentation on folders. In the worst case, names will actually push or shrink their widget when they get too long (flexbox).

These examples had all controllers in a single folder, presumably for the purpose of adding a title to the top of the panel. The folders seem unnecessary since lil-gui places the 'close controls' button on top, and that can be renamed. I moved these controllers to the root and moved the folder titles up to the panel title.

Couldn't test

I can't run the WebGPU examples or this one VR example :(

CSS for mobile

In examples/main.css I added styles for the mobile page that move the panel to the bottom left and prevent it from getting too big.

I wasn't sure of the purpose of this css but I carried it to .lil-gui

z-index: 2 !important; /* TODO Solve this in HTML */

lil-gui has logic that's supposed to prevent scrolling and dragging sliders simultaneously on mobile, but it still winds up happening in the three.js examples somehow. I think it has something to do with the iframe? I'm not able to reproduce that when I view the examples bare, or on the other test beds I've set up.

Non-module transform?

I didn't fully absorb how the rollup.examples script works, but I renamed the library to end with 'module.min.js' in this repo. I imagine the script needs something more? And is it correct to assume we want the minified lib?


So I couldn't bring myself to delete dat.gui outright. The last place it's referenced is tests/e2e/clean-page.js. Got a little sentimental. You know we're probably right around the 10 year mark of when we made it? Anyway, lemme know how this all looks. Thanks all!

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 31, 2021

I suspect the E2E test fail because removing the GUI from the examples works different with the new lib. This is the current approach:

/* Remove dat.gui and fonts */
const style = document.createElement( 'style' );
style.type = 'text/css';
style.innerHTML = `body { font size: 0 !important; }
#info, button, input, body > div.dg.ac, body > div.lbl { display: none !important; }`;
const head = document.getElementsByTagName( 'head' );
if ( head.length > 0 ) {
head[ 0 ].appendChild( style );
}

Is this still correct?

@georgealways
Copy link
Contributor Author

georgealways commented Oct 31, 2021

@Mugen87 ah! I didn't quite understand what that script was doing. I updated it so it targets .lil-gui instead of .dg.ac and the e2e tests all seem to run okay now locally, except for one: webgl_effects_ascii. That one seems to be running okay still? It didn't have a dat.gui...

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 1, 2021

That one seems to be running okay still?

Yes, looking good.

I can't run the WebGPU examples or this one VR example :(

WebGPU examples looking good, too. Tested webxr_vr_layers on a Quest 2 and it works as before.

And is it correct to assume we want the minified lib?

Yes, if possible it's preferable to have the minified lib in the repository.

I didn't fully absorb how the rollup.examples script works, but I renamed the library to end with 'module.min.js' in this repo. I imagine the script needs something more?

rollup.examples.config.js ignores the libs folder. I don't think it is necessary to add the examples/js (non-module) version of lil-gui.

@georgealways
Copy link
Contributor Author

nice, glad to hear those are working!

rollup.examples.config.js ignores the libs folder. I don't think it is necessary to add the examples/js (non-module) version of lil-gui.

Aha-- does rollup.examples just transform the non-core Three classes? I thought it was baking out a whole other module-free examples folder or something 😅

The only reason examples/js/libs has my attention is there's a non-module dat.gui in there as well. Should I be putting the UMD lil-gui in there?

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 1, 2021

The only reason examples/js/libs has my attention is there's a non-module dat.gui in there as well. Should I be putting the UMD lil-gui in there?

I'm not sure about @mrdoob'sc current preference here. When I remember correctly, we did not delete examples/js/libs/dat.gui.min.js for backward compatibility reasons, see #16948 (comment)

@mrdoob
Copy link
Owner

mrdoob commented Nov 1, 2021

Well, a lot of months have passed now. I think we should delete it 👋🥺

@georgealways
Copy link
Contributor Author

he shall enjoy a well earned retirement

@mrdoob
Copy link
Owner

mrdoob commented Nov 3, 2021

Thanks!

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 4, 2021

Well, a lot of months have passed now. I think we should delete it 👋🥺

Added this change to the migration guide just to be on the safe side 😅.

https://github.com/mrdoob/three.js/wiki/Migration-Guide#134--135

@georgealways
Copy link
Contributor Author

@Mugen87 @mrdoob Hey all, what's the procedure for updating the library on new releases? I don't see it being a super common thing, but these are the changes in the version I just published:

0.12.0

28.11kb, 8.24kb gzipped

  • Added support for dragging number fields vertically.
  • Fixed a bug that stopped you from entering decimals in number fields on mobile.
  • Fixed a bug that stopped screen readers from reading names in BooleanController.
  • Removed some dead CSS and JS.

The built source isn't in version control, but this is a diff of 0.11 to 0.12 if you want a closer look at the changes.

The vertical dragging for number fields makes them a lot more useful when there's not a slider. The total size is about the same since some dead code got stripped. The fork I've made for this original PR has the updated lib in place. Let me know!

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 9, 2021

Hey all, what's the procedure for updating the library on new releases?

We just upgrade the library in examples/jsm/libs/ with a separate PR. We don't do this for each release of a dependency but if you think it's worth in context of lil-gui please file a PR.

@georgealways
Copy link
Contributor Author

That makes sense! Unless there's an update we're screaming for, I'll throttle my PR's to be at most a "monthly-ish" thing. And hopefully much less often than that after this initial release period is over.

@mrdoob mrdoob added this to the r135 milestone Nov 26, 2021
performonkey added a commit to performonkey/three-fatline that referenced this pull request Dec 3, 2021
performonkey added a commit to performonkey/three-fatline that referenced this pull request Dec 3, 2021
performonkey added a commit to performonkey/three-fatline that referenced this pull request Dec 3, 2021
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