-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
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
TextGeometry changes and support in editor #27931
base: dev
Are you sure you want to change the base?
Conversation
- add support for TextGeometry in editor. currently this is linked only to a single font, but all of TextGeometry functionality is wired to the editor's sidebar UI, which I think is more than sufficient now - depreciating `height` parameter in TextGeometry options for `depth` for consistency with ExtrudeGeometry, which refers to the thickness the Text will be extruded. using `height` is still supported but will emit a warning in the console - also added a `scale` parameter in TextGeometry and editor. The numbers that work well for font sizes is unusally large in three.js objects (esp in VR), so passing 0.01 makes it more the conversion more pleasant to work with - Serialization and deserialization works. toJSON() and fromJSON() is added to TextGeometry. I have to add this to Geometries.js for scene loading to work automatically in editor, but the dependency of an addon (TextGeometry is addon, ExtrudeGeometry is core) seems a little strange. Either there's a better way to extend the supported Geometries deserialization list or perhaps move TextGeometry out of the examples
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
It seems it is not possible to save/reload a text mesh. The missing serialization/deserialization support was the main reason why Since When serialize/deserialize instances of geometry generators, their type should be retained. Meaning if you serialize an instance of |
@Mugen87 - threejs builds are not updated in his PR which is why the Text geometry serialization/deserialization is not working in linked example is because the The new TextGeometry I'll push the builds in the next commit. |
it appears the previous rawgit link is cached, but using this link should show TextGeometry persisting and loading from IndexedDB across refreshes. https://rawcdn.githack.com/zz85/three.js/33a29ddc4c1d3133d2e4082005f789d3269ed0b4/editor/index.html |
@zz85 Thanks, I've totally overlooked the changes to |
changes from core Geometries now moved to loader.registerGeometry( 'TextGeometry', TextGeometry ) https://rawcdn.githack.com/zz85/three.js/e646d485f0bcc2099ac2736f734231cb84f8ea6b/editor/index.html |
Add `@deprecated` annotation, update warning text.
@@ -35,7 +36,15 @@ class TextGeometry extends ExtrudeGeometry { | |||
|
|||
// translate parameters to ExtrudeGeometry API | |||
|
|||
parameters.depth = parameters.height !== undefined ? parameters.height : 50; | |||
if ( parameters.depth === undefined && parameters.height !== undefined ) { |
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 PR does two things in one go. It adds support to the editor and renames the height
parameter to depth
.
Ideally, there is a single PR for each change since both changes are independent of each other.
Besides, if height
is renamed, the example code (meaning webgl_geometry_text.html
and others) should be updated in order to avoid deprecation warnings.
That said, I also favor depth
since height
was a confusing name for describing the thickness (or depth) of the text.
Would you be okay with moving the renaming change to a separate PR?
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.
Sure I can do that. I'll wait on the confirmation of the loader.registerGeometry()
api before making the changes.
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.
actually the change to extract is easier to do, so it's in #27949
@@ -261,6 +263,12 @@ class ObjectLoader extends Loader { | |||
|
|||
} | |||
|
|||
registerGeometry( type, klass ) { |
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.
@mrdoob Are you okay with this kind of new interface?
This approach is in general interesting since it could be used in context of other classes like materials as well. Meaning custom/addon materials implement a static fromJSON()
(factory) method that ObjectLoader
/MaterialLoader
can use to create an instance. The internal material lib can be enhanced via:
loader.registerMaterial( 'MeshGouraudMaterial', MeshGouraudMaterial );
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.
It's interesting indeed...
I'd rename this.Geometries
to this.geometryTypes
though.
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.
I have updated ObjectLoader
manually but there still seems to be a diff in build/three.cjs
.
Apart from that, the PR looks good to me!
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.
I updated a merge from mrdoob/dev so there should be no conflicts now
I'll be out for the next week or so, so I removed the builds for this branch have it caught up with |
It seems there is still a diff though. |
sorry about that, should be fixed again |
Rename `this.Geometries` to `this.geometryTypes`.
The easiest way to support different fonts as it is today is to have a fonts dropdown list which all the typeface fonts in the examples directory could be loaded by The more comprehensive approach might be to have Editor keep a registry of all fonts, also allowing the user to drag and drop fonts (including ttfs). The serialization format in TextGeometry in this PR current state json-ify the entire font for convenience. I think it makes it easy for deserialization, but the approach to optimize for space would be to serialize fonts separately, then have TextGeometry look up the font database say via uuid. |
|
||
if ( scale !== undefined ) { | ||
|
||
this.computeBoundingBox(); |
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 is the bounding box computed at this point? It will be automatically recomputed when calling BufferGeometry.scale()
?
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.
BTW: Is the introduction of a scale
parameter really necessary? Couldn't apps just use Object3D.scale
instead?
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 reason to do it here is getting a scaled down (0.01) geometry mostly for connivence to be used in Editor.
the object.scale
could be used, but I wasn't sure if it was a good idea to insert a TextGeometry at a custom object scale, hence having it in the geometry seems like a more convenient way.
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.
In order to keep the parameters of TextGeometry
to a minimum, it would be better to use Object3D.scale
and not introduce an additional way for transforming geometry, imo.
I've noticed today that similar to #28187, published scenes with |
hmm, could we then just export addons by default, or exported them if TextGeometry is used?.. |
#28187 tried to conditionally add addons, imports and initialization code to published apps depending on the addon usage but I'm unsure about the implementation, tbh. |
Related issue: maybe
Description
Add text geometry support to three.js editor
** details **
height
parameter in TextGeometry options fordepth
for consistency with ExtrudeGeometry, which refers to the thickness the Text will be extruded. usingheight
is still supported but will emit a warning in the consolescale
parameter in TextGeometry and editor. The numbers that work well for font sizes is unusally large in three.js objects (esp in VR), so passing 0.01 makes it more the conversion more pleasant to work withThis contribution is funded by blurspline