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

Supporting text effects + fonts on Text Entities #16008

Merged
merged 2 commits into from Aug 2, 2019

Conversation

@HifiExperiments
Copy link
Contributor

commented Jul 30, 2019

https://highfidelity.atlassian.net/browse/DEV-335

http://roadmap.highfidelity.com/open-source-project-proposals/p/expose-more-options-for-text-entities

Test plan:

  1. Run this script: https://gist.githubusercontent.com/HifiExperiments/a855fcf3eb4c25cb3805700bea092b8f/raw/a868dadac2c68c6d3b1da6898bed0037df1eb667/TextEffectTest.js
  2. You'll see:
    image
  3. Press "o" to toggle the textAlpha. The text should still be colored correctly (in master, it turns black, it'll look more grey now). Press "o" again to go back to opaque.
  4. Press "u" to cycle through the fonts. In order, the fonts will go: Roboto, Courier, Timeless, Inconsolata, Roboto (invalid font), Roboto (empty font), Timeless (but downloaded from an http link)
  5. Press "i" to cycle through the text effects: none, outline, outline fill, shadow. You'll see the behavior at various textEffectThickness:
    image
    image
    image
    The outline effect suffers from aliasing due to the thin lines, but the effect improves as you get closer or use larger textEffectThicknesses:
    image
    I've also left comments in the code with recommendations on how to improve that aliasing in the future.
@hifi-gustavo

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

Can someone from High Fidelity approve this PR?

1 similar comment
@hifi-gustavo

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

Can someone from High Fidelity approve this PR?

@CLAassistant

This comment has been minimized.

Copy link

commented Jul 30, 2019

CLA assistant check
All committers have signed the CLA.

@ZappoMan
Copy link
Contributor

left a comment

We definitely like the sounds of this. We were doing some internal work similar to this.

I'm going to "project approve" this for 84, but cc: @zfox23 and @MiladNazeri who may have another related PR in the works.

@MiladNazeri MiladNazeri added the JSDocs label Jul 30, 2019

@MiladNazeri

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

Looks really good, some parts I couldn't follow re: the shader logic, but I think another engine eye is going to take a look.

@MiladNazeri

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

Adding this comment here from #16007 just in case:

@HifiExperiments looking at #16008, one thing I'm not sure of to make this API work without needing to create an actual text entity first, is where I can set the font size if I can't do that in getInstance anymore. I only see a getFontSize I can access.

@shanzzam

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

Putting the DNM label on this until @samcake can take a look at the wording of it.

@samcake
Copy link
Contributor

left a comment

Good to go
We will need to keep an eye on the performances as the new options of the text shaders are more expensive., especially on low end

@MiladNazeri

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

I'm adding pre-merge testing on this one based on @samcake's comment.

@MiladNazeri

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

build this please

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@HifiExperiments HifiExperiments force-pushed the HifiExperiments:text branch from 931cdc3 to 5b1ddec Aug 2, 2019

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@MiladNazeri

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

build this please

@HifiExperiments HifiExperiments force-pushed the HifiExperiments:text branch from 5b1ddec to 029a494 Aug 2, 2019

@MiladNazeri

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

build this please

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

Android build is available here. Quest build is available here

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@shanzzam

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

Ready for testing.....

@salinacsoto

This comment has been minimized.

Copy link

commented Aug 2, 2019

  1. When toggling on step 3 of the textAlpha the text flashes white.
  2. @HifiExperiments your example photos look different than what we see. Yours look like there is some kind of lighting. If we tried to do these tests in the void like you did everything is too dark and we can't test step 5.
    I guess my question is, should we set up a certain kind of lighting for testing? Or is there something wrong with the script?
@salinacsoto

This comment has been minimized.

Copy link

commented Aug 2, 2019

hifi-snap-by-skelina-on-2019-08-02_12-07-56

@HifiExperiments

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

@salinacsoto that flashing is a result of a frame of lag when switching between transparent and opaque. I think it happens with other entity types too, I wouldn’t consider it a blocker.

As for the lighting, the script makes the text so that it’s facing you. If you run the script while the keylight is behind you, it’ll be lighting the front of the text, like in my pictures.

@salinacsoto

This comment has been minimized.

Copy link

commented Aug 2, 2019

Alright :) I am going to give this a pass then

@zfox23

zfox23 approved these changes Aug 2, 2019

@GayathriKamath
Copy link
Contributor

left a comment

re-approving!

@MiladNazeri MiladNazeri merged commit ae7b415 into highfidelity:master Aug 2, 2019

2 checks passed

default Build finished.
Details
license/cla Contributor License Agreement is signed.
Details

@HifiExperiments HifiExperiments deleted the HifiExperiments:text branch Aug 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.