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

Updates to ISLCObjectNode's label text #79

Closed
zepumph opened this issue Sep 9, 2019 · 26 comments
Closed

Updates to ISLCObjectNode's label text #79

zepumph opened this issue Sep 9, 2019 · 26 comments

Comments

@zepumph
Copy link
Member

zepumph commented Sep 9, 2019

From work instrumenting GFL for PhET-iO, there are some new problems with the label text for the ISLCObjectNode. Originally the goal was to instrument both the label and shadow text as a single element, but that added too much complexity, and caused phetsims/gravity-force-lab-basics#172. So as a result in phetsims/gravity-force-lab#175 we decided to try to add a stroke around the label, and to get rid of the shadow. Here are the following problems that we are trying to support:

  1. Appropriate sizing in all three ISLC sims (aka fixing Mass strings are tiny gravity-force-lab-basics#172)
  2. Accenting the white text so that it still has good contrast for all mass/charge values in all three sims.
  3. A good maxWidth so that in studio, GFL (which has a label like m1) can be changed to a longer string without scaling it strangely.

Currently 2 and 3 are in direct opposition to each other. Allow me to explain.

Ideally maxWidth wouldn't be used to clamp the size down of the original string, but instead used to provide a good maximum for i18n (i.e. longer strings). This isn't the case in the current code. maxWidth is setting the size or the english string, such that the PhetFont is irrelevant;no matter the font size, the maxWidth sets the size back down to the size of the english string. To fix the immediate problem, I increased the maxWidth, and then set the PhetFont accordingly for each sim, generally making them smaller, so that they match the current size of labels. When doing this, we lose the ability to boost the contrast using stroke. This is because the minimum lineWidth for a stroke around the font size needed is too strong. In GFLB PhetFont of 11 works well, but the minimum stroke width looks like this:
image

So now it seems like we are stuck between supporting a larger maxWidth for phet-io and i18n, and accenting the text with a stroke. I'll keep investigating.

@zepumph
Copy link
Member Author

zepumph commented Sep 9, 2019

The issue outlined in the above screenshot was because lineWidth was not supported by RichText. I added it over in phetsims/scenery#998, and that is out for review. Note that there are a few artifacts on the tops of letters, I asked @jonathanolson for his opinion on those over in that issue.

@ariel-phet @arouinfar please review how master looks for the three sims, and let me know what you think about the stroke, especially the line width of it. I think that it is a little light for contrast's sake. But any darker looked pretty weird. I would also recommend going into gravity-force-lab studio and changing the mass1Node.labelText.textProperty to see it change, scale, and center.

@zepumph
Copy link
Member Author

zepumph commented Sep 9, 2019

Also note that the texts should now be back to their original size (re phetsims/gravity-force-lab-basics#172), and confirming that would be good to do also.

@ariel-phet
Copy link

@zepumph, for lack of a better term, the current text on master looks kind of "janky" and a bit blurry, I don't think it is a reasonable solution yet.

I suppose I don't know exactly what TextWithShadow does, but would it be possible using RichText to just layer a black letter under the white letter, slightly displaced? Is that what TextWithShadow already did?

@ariel-phet ariel-phet removed their assignment Sep 11, 2019
@zepumph
Copy link
Member Author

zepumph commented Sep 11, 2019

TextWithShadow took two texts, one white and one black, and put them on top of each other, with the black one on bottom and a little offset. The issue was that then we have two texts to instrument and keep in sync. I'm not really sure what the general solution here is. I'm going to mark for developer meeting to see if others have an idea.

For developers, the goal is to have a shadow-like text, but for PhET-iO concerns it is easiest for it to be one text. Is this possible?

@arouinfar
Copy link

arouinfar commented Sep 11, 2019

@zepumph thanks for getting the stroked text up and running! I generally agree with @ariel-phet.

When the browser window is maximized, the outline looks pretty good. However, the text is harder to read at smaller resolutions, and doesn't scale very well for long strings (even in a maximized window).

The shadow text is easier to read when the text is small.
image
image

@arouinfar arouinfar removed their assignment Sep 11, 2019
@zepumph
Copy link
Member Author

zepumph commented Sep 11, 2019

@pixelzoom had a helpful comment over in phetsims/scenery#998 (comment):

Imo, stroking a font (in general) looks lousy. You'd be better off filling bold text with a color that looks good on each sphere, or putting a translucent background behind filled text.

@arouinfar
Copy link

You'd be better off filling bold text with a color that looks good on each sphere

In principle, I agree, but that's not possible here. With the Constant Size option, the color varies from nearly white to fully saturated red or blue. Black text would look best for low densities, but becomes unreadable when the color of the mass is fully saturated. White text would look best for high densities, but would be impossible to read at the lower end of the spectrum.

translucent background behind filled text

I'd be fine with giving this a try to see how it looks. I think black text with a transparent white background would be ideal.

@samreid
Copy link
Member

samreid commented Sep 12, 2019

We should continue the development of SCENERY_PHET/ShadowText.js and instrument it (without instrumenting the inner 2 Text instances--they are an implementation detail). We should strive to give our Text types similar PhET-iO interfaces: RichText, ShadowText, Text (maybe others?) should have setText, setFontOptions, setMaxWidth, etc.

@jonathanolson
Copy link
Contributor

Can we just have a view Property for the text content, that both of the Text nodes follow? Or have the client responsible for setting both?

@zepumph
Copy link
Member Author

zepumph commented Sep 12, 2019

Thanks for the ideas. I don't think this needs any more developer feedback. I'm going to try a translucent background next, and see what designers think. If that doesn't pan out we can look at spending more time on TextWithShadow.js. The reason we abandoned it quickly was because there weren't really any other usages of the general shadow text.

@zepumph
Copy link
Member Author

zepumph commented Sep 12, 2019

@ariel-phet @arouinfar how does this look? I added a transparent background.

image

It is pushed to master so you can play with it in GFL. A few notes about the implementation:

  • the opacity is currently at .5, in GQ there was a similar implementation that was set to .75. What do you think?
  • We can adjust the margins and corner radius as you see fit. Just let me know.
  • The font is currently bold.
  • Any other thoughts?

zepumph added a commit that referenced this issue Sep 12, 2019
@arouinfar
Copy link

@zepumph I'm liking the transparent background! The padding, corner radius, and bold font all look good to me. I would like to bump the opacity up to 0.6, to help boost the contrast of the text when the color is fully saturated.

As an aside, I don't love how the labels spill over a bit in constant size mode in GFLB. Could we bump up the size of the masses just a tiny bit? If the size in GFL and GFLB are the same, the change could apply to both sims. @zepumph if you'd like to move this request to a separate issue, please let me know.

image

@arouinfar
Copy link

What about if we just move the label off the masses, and just have it "stick" a fixed distance from the bottom center of the masses? It seems like that will allow for a longer strings, plenty of flexibility (and slightly more complicated code...)

@ariel-phet @zepumph I like that idea! I would keep the text the same, but relocate it to the bottom of the mass. The label positioning in @ariel-phet's mockup looks good.

Another way we can optimize for longer strings, would be to have smart labels that shift away from one another when the masses are close. This certainly isn't critical, but would be a nice touch.

I could see a client wanting to change the labels on the charges to something like charge 1 and charge 2, which will become unreadable when the masses are close.
image

@arouinfar arouinfar removed their assignment Sep 18, 2019
@arouinfar
Copy link

arouinfar commented Sep 18, 2019

@zepumph I saw this sim went up for rc in phetsims/qa#429 (exciting!), but I think we'll want to include the label positions suggested by @ariel-phet in #79 (comment) in the 1.0 release.

The label shifting to avoid overlaps suggested at the end of #79 (comment) isn't necessary for 1.0. If we decide to go there, I think it could get folded into the PhET-iO work.

@zepumph
Copy link
Member Author

zepumph commented Sep 19, 2019

Agreed! I was expecting to patch in the final touches to this issue, but I wanted to get that test going sooner.

@zepumph
Copy link
Member Author

zepumph commented Sep 20, 2019

I added the labels below the object in the above commit. This involved small tweaks to the default ruler positions. It is possible, like @arouinfar stated above, to have labels overlap under certain conditions with longer labels specified by PhET-iO. It doesn't bother me too much though.

@arouinfar @ariel-phet how does this look?

zepumph added a commit to phetsims/coulombs-law that referenced this issue Sep 20, 2019
@zepumph zepumph assigned ariel-phet and arouinfar and unassigned zepumph Sep 20, 2019
@ariel-phet
Copy link

@zepumph looking pretty good to me. Just one minor tweak:

  • In Coulomb's Law, Macro Scale screen, I would center the ruler vertically in the negative space (right now, default position is a little too close to the charge labels)

@ariel-phet ariel-phet assigned zepumph and unassigned ariel-phet Sep 23, 2019
@arouinfar
Copy link

The labels look great @zepumph.

In Coulomb's Law, Macro Scale screen, I would center the ruler vertically in the negative space (right now, default position is a little too close to the charge labels)

Agreed, I would shift the ruler down to about here:

image

@arouinfar arouinfar removed their assignment Sep 23, 2019
@zepumph
Copy link
Member Author

zepumph commented Sep 23, 2019

Thanks. Implemented above. I'll cherrypick what's needed into the rc now. Closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants