search

EtherealEngine / etherealengine

iR Engine - Bringing us together on the open social spatial web. 🤖 🚀 👓 🕹ī¸ 🧑đŸŋ‍🚀
https://etherealengine.org
Other
702 stars 247 forks source link

IR-2771: Update onSet to correctly read colors #10443

Closed CITIZENDOT closed 1 week ago

CITIZENDOT commented 1 week ago

Summary

Subtasks Checklist

Breaking Changes

References

closes #insert number here

QA Steps

heysokam commented 1 week ago

@CITIZENDOT I see no NodeEditor changes in the PR, so that raises some questions for me: note: they might be because of me having a knowledge gap, so its not a review just a question

  1. Does the editor understand number type colors?
  2. If that's not the usecase for the change, in which case would we expect the user to send the color as a number instead of a Color object?
  3. If the usecase is only to programmatically change colors, wouldn't we want to expose support for string as a valid input type too, since Troika also supports that format? (see the TroikaTextColor type union in our code)
CITIZENDOT commented 1 week ago

  1. I think Editor does understand colors when they're used as number type. For eg: DirectionalLightComponent's onSet is reading color property whether it's of string type or number type alike. This component is also initialised with new Color(): https://github.com/EtherealEngine/etherealengine/blob/9ff957ca84ae104dbf66b2d2cc8b18b71995a2f6/packages/spatial/src/renderer/components/DirectionalLightComponent.ts#L121

  2. onSet is receiving json.fontColor as a number. I think this has something to do with ColorInput. I'm not sure. On dev, fontColor is only being set if it's a Color object. Hence, it's not being updated when user is changing it.

  3. I don't think we want to support a string type. We are using Color type because it's a native three js type, and number type because that's how we're receiving JSON data. But I don't see a reason why we want to support string type. But also I'm not sure. If there's any reason to support it, We can add it here.

Btw, I couldn't find TroikaTextColor, can you link it here?

heysokam commented 1 week ago

@CITIZENDOT

  1. Makes sense, ty ty
  2. I didn't notice that the color wasn't changing when I implemented it. Good catch, ty!
  1. [...] But I don't see a reason why we want to support string type. But also I'm not sure. If there's any reason to support it, We can add it here.

I don't see a reason either, other than giving the user the option to send a color as a string programatically. But then I believe we would need to validate the string? Unless troika already validates it, never checked. But yeah, programatically setting the color with a #001122FF format, and similars, would be the only reason I can think of. Unless its a really easy change (meaning not requiring validation) I guess we can add it later if we need it :shrug:

Btw, I couldn't find TroikaTextColor, can you link it here?

sorry, I misremembered the name of the type. its TroikaColor, at the very top of TextComponent.ts https://github.com/EtherealEngine/etherealengine/blob/9ff957ca84ae104dbf66b2d2cc8b18b71995a2f6/packages/engine/src/scene/components/TextComponent.tsx#L45