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

Polish text bitecs #5933

Merged
merged 2 commits into from
Mar 15, 2023
Merged

Polish text bitecs #5933

merged 2 commits into from
Mar 15, 2023

Conversation

takahirox
Copy link
Contributor

This commit pilishes text bitecs. The changes are

  • Rename Text component to TextTag to avoid the confliction with the WebAPI Text
  • Rewrite inflators/text from JavaScript to TypeScript
  • Explicitly declare text inflator params
  • Add Text system that calls sync() and remove sync() calls from other systems
  • Update type/troika-three-text.d.ts to match the troika-three-text API
  • Fix some bugs

Copy link
Contributor

@johnshaughnessy johnshaughnessy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Thanks for adding all of the type infos

lineHeight?: number | "normal";
maxWidth?: number;
opacity?: number;
outlineBlur?: number | `${number}%`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never seen this type definition before. What does ${number}% mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be similar to regex. (If I understand correctly) ${number}% means number string that ends with "%", like "25%" or "0.5%".

https://www.typescriptlang.org/docs/handbook/2/template-literal-types.html#handbook-content
https://stackoverflow.com/questions/51445767/how-to-define-a-regex-matched-string-type-in-typescript

Copy link
Contributor

@johnshaughnessy johnshaughnessy Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know about this before. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know that was supported either. Neat!

text.strokeWidth = params.strokeWidth!;
text.textAlign = params.textAlign!;
text.textIndent = params.textIndent!;
text.whiteSpace = params.whiteSpace!;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed, we might be able to get rid of the !s by defining two types:

OptionalTextParams where each of the properties is optional and TextParams where each property is required.

The function signature becomes:

export function inflateText(world: HubsWorld, eid: number, params: OptionalTextParams) {

The defaults become

const DEFAULTS: TextParams = {

And in the body of the inflator we write:

const finalParams : TextParams = Object.assign({}, DEFAULTS, params)

Copy link
Contributor Author

@takahirox takahirox Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I experimented. It works (type check passes).

But some disadvantages.

  1. Many duplicated type declaration
  2. We need to carefully manage that type declarations are synched. Even if types are different between them (ex: string / number) type check didn't catch.

So, I would like to suggest to experiment more to improve the idea, and make another PR for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typescript has a generic utility type to make all the fields of a type optional (or required) https://www.typescriptlang.org/docs/handbook/utility-types.html#partialtype - Worth looking through the other types in that doc too just to know what tools are available.

So we could either do:

export function inflateText(world: HubsWorld, eid: number, params: Partial<TextParams>) {

or

const finalParams = Object.assign({}, DEFAULTS, params) as Required<TextParams>;

(I think the Partial one is more natural)

Copy link
Contributor

@netpro2k netpro2k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for cleaning that up!

// sync() checks whether to invoke costly processing
// inside.
//
// sync() call can happen one frame after that text
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We choose when to run this system, I think we should just enforce this guarantee.

// a big deal because what sync() invokes is async
// processing.
//
// Question: Is it safe even if text object is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This query won't match on removed entities, and the text wont be disposed of until the removeObejct3dSystem runs (or in this system once we refactor where we do cleanup). So it should always be safe to call this. I assume TriokaText takes care of disposing of any in flight rendering when you dispose of a text.

value: string;
anchorX?: "left" | "center" | "right";
anchorY?: "top" | "top-baseline" | "top-cap" | "top-ex" | "middle" | "bottom-baseline" | "bottom";
clipRect?: [number, number, number, number] | null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to support both null and undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't "?" imply that accepts undeinfed?


// Defaults values must be set for all the optional properties
// to safely use foo! operator in the inflator.
const DEFAULTS: TextParams = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I like about using Partial/Required as discussed below is this would be type checked for us so we couldn't accidentally miss a default value.

@@ -245,7 +246,8 @@ export function mainTick(xrFrame: XRFrame, renderer: WebGLRenderer, scene: Scene
hubsSystems.gainSystem.tick();
hubsSystems.nameTagSystem.tick();
simpleWaterSystem(world);

textSystem(world);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its very crude but we may want to add a blank line above this system to make it clear its a bit "special". Or maybe better to just add a comment like // all systems that update text properties should run before this

This commit polishes text bitecs. The changes are

* Rename Text component to TextTag to avoid the confliction
  with the WebAPI Text
* Rewrite inflators/text from JavaScript to TypeScript
* Explicitly declare text inflator params
* Add Text system that calls sync() and remove sync() calls
  from other systems
* Update type/troika-three-text.d.ts to match the
  troika-three-text API
* Fix some bugs
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.

None yet

3 participants