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
| 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
node-graph/gcore/src/text.rs Outdated
| // 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
|
adamgerhant force-pushed the typography branch from b4eb01f to fe7766b Compare 
timon-schelling force-pushed the master branch from 124235a to a42cad8 Compare Keavon force-pushed the master branch 6 times, most recently from d6228da to e58c1de Compare Keavon force-pushed the master branch 4 times, most recently from fcc53f5 to 9b97ab7 Compare 2 participants
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