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

Examples: Hide scrollbars in webgl_effects_ascii. #25047

Merged
merged 5 commits into from
Dec 9, 2022

Conversation

linbingquan
Copy link
Contributor

@linbingquan linbingquan commented Nov 30, 2022

Related issue: #XXXX

Description

  1. Add CSS to hidden the scrollbar for body.
Before After
image image
  1. Used OrbitControls to replace TrackballControls.

I feel OrbitControls is easier to use than TrackballControls.
TBH, I rarely use TrackballControls.
If there is a problem, I will cancel the second change.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 30, 2022

I think there is no need to replace the controls. The CSS fix is of course good.

@WestLangley
Copy link
Collaborator

WestLangley commented Nov 30, 2022

Yes, I think the controls should be replaced with OrbitControls. TrackballControls is not appropriate for this scene.

Then, controls.update() is not needed. (Or retain it and add damping.)

@linbingquan
Copy link
Contributor Author

The CSS fix is of course good.

I just only keep CSS fix, I think this PR will be easier to merge.

I think there is no need to replace the controls.
I think the controls should be replaced with OrbitControls.

Thank you for your reply, the controls code is a little controversial. Let's keep it the same.

@Mugen87 Mugen87 added this to the r148 milestone Dec 1, 2022
@mrdoob
Copy link
Owner

mrdoob commented Dec 2, 2022

Doing body { overflow: hidden; } tends to not be the right solution, but it's a way to hide the problem.

We used to have that in all the examples until we realized the proper solution was doing canvas.style.display = 'block'; inside WebGLRenderer.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 2, 2022

I've tried that but unfortunately it does not work with AsciiEffect. Depending on the effect's options resolution and scale, the domElement might differ in size which makes it impossible to match it to the defined size via setSize().

@Mugen87 Mugen87 changed the title Examples: Improved webgl_effects_ascii.html Examples: Hide scrollbars in webgl_effects_ascii. Dec 6, 2022
@mrdoob
Copy link
Owner

mrdoob commented Dec 7, 2022

This seems to do the trick...

diff --git a/examples/jsm/effects/AsciiEffect.js b/examples/jsm/effects/AsciiEffect.js
index 1ad06a4b45..af5eb7f02e 100644
--- a/examples/jsm/effects/AsciiEffect.js
+++ b/examples/jsm/effects/AsciiEffect.js
@@ -59,8 +59,8 @@ class AsciiEffect {
 
 		function initAsciiSize() {
 
-			iWidth = Math.round( width * fResolution );
-			iHeight = Math.round( height * fResolution );
+			iWidth = Math.floor( width * fResolution );
+			iHeight = Math.floor( height * fResolution );
 
 			oCanvas.width = iWidth;
 			oCanvas.height = iHeight;
@@ -81,9 +81,6 @@ class AsciiEffect {
 			oAscii.cellPadding = 0;
 
 			const oStyle = oAscii.style;
-			oStyle.display = 'inline';
-			oStyle.width = Math.round( iWidth / fResolution * iScale ) + 'px';
-			oStyle.height = Math.round( iHeight / fResolution * iScale ) + 'px';
 			oStyle.whiteSpace = 'pre';
 			oStyle.margin = '0px';
 			oStyle.padding = '0px';
@@ -251,7 +248,7 @@ class AsciiEffect {
 
 			}
 
-			oAscii.innerHTML = '<tr><td>' + strChars + '</td></tr>';
+			oAscii.innerHTML = `<tr><td style="display:block;width:${width}px;height:${height}px;overflow:hidden">${strChars}</td></tr>`;
 
 			// console.timeEnd('rendering');
 

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 7, 2022

Cool! I can confirm the scroll bars go away with these changes, too!

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 9, 2022

@linbingquan Would you mind updating the PR according to @mrdoob's suggestion?

@linbingquan
Copy link
Contributor Author

Would you mind updating the PR according to @mrdoob's suggestion?

I'm willing to give it a try.

I found body { overflow: hidden; } is unnecessary with mrdoob's suggestion code.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 9, 2022

I found body { overflow: hidden; } is unnecessary with mrdoob's suggestion code.

Yes, so the modifications to the example can be reverted.

@linbingquan
Copy link
Contributor Author

Yes, so the modifications to the example can be reverted.

Done.

@Mugen87 Mugen87 merged commit 9d332c1 into mrdoob:dev Dec 9, 2022
@linbingquan linbingquan deleted the dev-examples-ascii branch August 19, 2023 11:21
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.

4 participants