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

HTMLMesh: added textNode wrapping #24781

Closed
wants to merge 3 commits into from

Conversation

jrjdavidson
Copy link
Contributor

Description

If you look at the video on #24779 , the textNode doesn't wrap properly. This PR fixes that.
image

PS my pico neo isn't able to run the 360 video- I hope that webxr layers will solve that (my headset is a pico neo 3 so haven't been able to test that..)

@mrdoob mrdoob changed the title added textNode wrapping HTMLMesh: added textNode wrapping Oct 19, 2022
@mrdoob mrdoob added this to the r146 milestone Oct 19, 2022
@mrdoob mrdoob modified the milestones: r146, r147 Oct 27, 2022
@jrjdavidson jrjdavidson marked this pull request as draft November 13, 2022 20:43
@jrjdavidson
Copy link
Contributor Author

Just noticed that this fix breaks for inline elements (e.g. span), will try to fix this later

@mrdoob mrdoob modified the milestones: r147, r148 Nov 30, 2022
@mrdoob mrdoob modified the milestones: r148, r149 Dec 22, 2022
@mrdoob mrdoob modified the milestones: r149, r150 Jan 26, 2023
@mrdoob mrdoob modified the milestones: r150, r151 Feb 23, 2023
@mrdoob mrdoob modified the milestones: r151, r152 Mar 30, 2023
@vincentfretin
Copy link
Contributor

I made a different implementation of it in AdaRoseCannon/aframe-htmlmesh#11
good enough for my use case. I don't plan to work on it further for now, but if you want to compare or do a better implementation, feel free to look at it.

@mrdoob
Copy link
Owner

mrdoob commented Apr 26, 2023

@vincentfretin Would you like to do a PR with your implementation?

@mrdoob mrdoob removed this from the r152 milestone Apr 26, 2023
@vincentfretin
Copy link
Contributor

Sure, I'll do a PR but probably with a slightly modified version in a few weeks when I'll take the time to do more tests with other languages like Japanese like I commented and double check again the hack about the font size I mentioned there. If you or anyone else have some insights about the comments on the PR AdaRoseCannon/aframe-htmlmesh#11 I'll be glad to read it.

@jrjdavidson
Copy link
Contributor Author

I remember struggling with implementing this as the textnode width in pixels differs from the parent element width on some displays- this led to some some wrong results when measuring the text width. Using the range instead of the context to compute the wrapping width helped solve this. I couldn't figure out why this was the case at the time.

@vincentfretin
Copy link
Contributor

When you say context, you mean ctx.measureText?
That's also the issue I'm seeing on Quest browser, that's why I multiplied the parent width by 1.01 arbitrarily when I'm doing the check if the text overflow or not.

@jrjdavidson
Copy link
Contributor Author

Yes that is what I mean. From memory, I didn't have any significant issues on the headset, but there was a 30% discrepancy (1.3 x the length) on my desktop screen. I might have used a different method though, and your implementation might avoid this. I can't check now but could have a look next week?

@vincentfretin
Copy link
Contributor

30% that was a lot, me it was 1% but I tested only between Quest browser and Firefox and Chrome on Ubuntu 22.04, also it may be related to the font used.
You can grab my code in the PR and let me know, you can play with the coefficient there. The HTMLMesh.js in the PR was based on r151 so before image support, there is comment at the top of the file.

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