Add the new Typography graphical type #3152
Conversation
| impl serde::Serialize for Graphic { | ||
| fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> | ||
| where | ||
| S: serde::Serializer, | ||
| { | ||
| let default: Table<Graphic> = Table::new(); | ||
| default.serialize(serializer) | ||
| } | ||
| } | ||
|
|
||
| impl<'de> serde::Deserialize<'de> for Graphic { | ||
| fn deserialize<D>(_deserializer: D) -> Result<Self, D::Error> | ||
| where | ||
| D: serde::Deserializer<'de>, | ||
| { | ||
| Ok(Graphic::Graphic(Table::new())) | ||
| } |
There was a problem hiding this comment.
So you just decided to not do any Serialization? Is there any reason to do this? This is a very confusing design decision and will likely lead to bugs in the future
There was a problem hiding this comment.
The Layout within the Typography cannot be serialized. Also its not meant to be a Tagged value, as it can only be produced at runtime by the text node. It only has this implemented since Graphic needs a tagged value for layer inputs. Similar to the wgpu::Texture.
There was a problem hiding this comment.
You have just successfully disabled serialization / deserialization for all Grahic data not just Typography
There was a problem hiding this comment.
Graphic data can never be serialized since it should never be anything other than an empty table (There is no widget to modify it)
|
Did you mean to mark this as ready for review? This pr does not seem to be finished |
Keavon
changed the title
Yes, this is only for use in the native node graph overlay node. There is no user integration yet to keep the scope small. |
| impl serde::Serialize for Graphic { | ||
| fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> | ||
| where | ||
| S: serde::Serializer, | ||
| { | ||
| let default: Table<Graphic> = Table::new(); | ||
| default.serialize(serializer) | ||
| } | ||
| } | ||
|
|
||
| impl<'de> serde::Deserialize<'de> for Graphic { | ||
| fn deserialize<D>(_deserializer: D) -> Result<Self, D::Error> | ||
| where | ||
| D: serde::Deserializer<'de>, | ||
| { | ||
| Ok(Graphic::Graphic(Table::new())) | ||
| } |
There was a problem hiding this comment.
You have just successfully disabled serialization / deserialization for all Grahic data not just Typography
| fn render_complexity(&self) -> usize { | ||
| 1 | ||
| } |
There was a problem hiding this comment.
This should depend on the text properties since text can be pretty perf intensive to render
There was a problem hiding this comment.
Id need to benchmark it, but given that rendering text with elements is around 100x faster than the text -> vector -> bezpath -> svg string conversion, I don't think it would be significant. Something based on the height/width could work though.
| impl PartialEq for Typography { | ||
| fn eq(&self, _other: &Self) -> bool { | ||
| true | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a dummy implementation required by TaggedValue::Graphic. It should never be called since a Graphic table should never be compared. a warn! message in here would be useful
There was a problem hiding this comment.
In that case Graphic should be removed from the Tagged Value enum, but this implementation is just non-sensical
| // Hash the Font for a unique id and add it to the cached hash | ||
| vacant_entry.key().hash(&mut hasher); | ||
| let hash_value = hasher.finish(); | ||
| self.hash = self.hash.wrapping_add(hash_value); |
There was a problem hiding this comment.
Instead of performing an addition here, you should hash the font data + the previous hash value. FxHasher is very fast but not particularly collision resistant and this could cause issues. There is also the question of why you use FxHasher anyway, it can't really be for perf reasons since you do also compute the hash of the same data using the default hasher. If you were concerned about performance we should only compute the hash once and reuse it for both use cases but I can guarantee you that the perf is going to be completely irrelevant in this case so idk.
If your intent was to make sure the insertion order does not affect the hash value, that is fine in principle but should an explaining commend and a justification why this does not create collisions
There was a problem hiding this comment.
The idea is to create a hash key representing the unique state of all current fonts in the NewFontCache. This ensures the SNI of the tagged value input which supplies the cache is updated when the cache changes, and is also very fast/does not require hashing the entire font cache. I thought the FxHasher is the default hasher that is generally used, since its used in NodeNetwork and to calculate stable node ids.
There was a problem hiding this comment.
The issue is that you are adding the hash codes that does not necessarily preserve the collision resistance. And FxHasher has a pretty low collision resistance to begin with so this likely amplify the issue. If you care about the position independence (and I don't think that we actually do) you could use the DefaultHasher here since it generates higher quality hashes and the amount of data we has is very small so perf should not be an issue. We only really use FxHasher if we expect to hash large amounts of data. If we cared more about collision resistance and still wanted to use FxHasher we could use a Diffie-Hellman group construction instead here by raising the generator group to the power of the hash value. But that would be a bit overkill since we don't really work in adversarial scenarios
adamgerhant
force-pushed
the
typography
branch
from
08b25f6 to
b4eb01f
Compare
Performance Benchmark Results
|
Adds the Typography data type, which is part of the Graphic enum and rendered to svg as
<Text/>and rendered to Vello withdraw_glyphsTODO: Replace
FontCachewithNewFontCache, update Text node to output Typography