Skip to content

Re-export app_units::Au or disable serde for app_units #3045

Open
@fschutt

Description

@fschutt
Contributor

In order to create a AddFontInstance struct, you have to use an app_units::Au. This is very annoying because:

  • app_units depends on serde and num_traits, both very compile-time heavy crates that are completely unused, yet they still need to be compiled in
  • This is the only place in the entire public webrender API where Au is used
  • app_units isn't re-exported, so other crates have to add the exact version that webrender uses to their Cargo.toml manually.

So my proposal is to create a function that creates an Au from an i32 value, this way you don't have to re-export the Au, i.e.:

pub fn webrender_create_au_from_px(value: i32) -> Option<Au> {
    let value = value * AU_PER_PX;
    if value < MIN_AU || value > MAX_AU {
        None
    } else {
        Some(Au(value))
    }
}

And please make serde and num-traits in the app-units crate optional - for a regular webrender application, these crates go completely unused because serde serialization is only used for the debugger, but these crates still have to be compiled, leading to bad compile times.

Activity

pyfisch

pyfisch commented on Sep 13, 2018

@pyfisch
Contributor

Why isn't it possible to replace Au with a a f32 and have a separate "size key"?

// The font size is in *device* pixels, not logical pixels.
// It is stored as an Au since we need sub-pixel sizes, but
// can't store as a f32 due to use of this type as a hash key.

The note states that an f32 can't be used as a hash key but we could require a synthetic key of type i32 for size instead. The caller would be responsible that no two f32 sizes map to the same size key. An easy way to achieve this is to transmute the f32 bitpattern to i32.

kvark

kvark commented on Sep 13, 2018

@kvark
Member

@pyfisch Au is not only giving us hash-ability and eq-ability, it's also binning the sizes, so that we don't end up creating two distinct font atlases for sizes that are only different by 0.00001.

fschutt

fschutt commented on Sep 14, 2018

@fschutt
ContributorAuthor

So I've just tried, but webrender is tied heavily to bincode and need serialization for that. It's not possible to turn off serde, which was my original goal, because it's necessary for serde-bincode - but it is possible to turn off num-traits although that doesn't help all that much.

However, re-exporting the app_unit types shouldn't be an issue. I would re-export it in webrender_api::units, so that webrender and wrench don't have to declare a separate version of app_units and can use webrender_api::{Au, AU_PER_PX, MIN_AU, MAX_AU}.

added a commit that references this issue on Sep 14, 2018

Auto merge of #44 - fschutt:master, r=jdm

7183948
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @kvark@pyfisch@fschutt

        Issue actions

          Re-export app_units::Au or disable serde for app_units · Issue #3045 · servo/webrender