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

Only respect the text color's alpha component for Bitmap glyphs. #1905

Closed
wants to merge 1 commit into from

Conversation

@mstange
Copy link
Contributor

mstange commented Oct 22, 2017

Fixes #1780.
Fixes #1876.

This restores a chunk of code that was added in #1754 and removed in #1816, with the difference that I use color.a as the alpha component instead of 1.0.


This change is Reviewable

@mstange
Copy link
Contributor Author

mstange commented Oct 22, 2017

r? @glennw

@Gankra
Copy link
Contributor

Gankra commented Oct 22, 2017

In the context of AppleColorEmoji, this change seem reasonable and correct.

However a big part of #1816 was refactoring how bitmap fonts actually work. Was that all just for that one font? Are there other fonts that this mode now applies to and wouldn't be correct?

FontRenderMode::Subpixel |
FontRenderMode::Alpha |
FontRenderMode::Mono => *color,
};

This comment has been minimized.

@Gankra

Gankra Oct 22, 2017

Contributor

As a minor style point, this could just be a ternary:

let color = if font.render_mode == FontRenderMode::Bitmap { ... } else { ... };

I don't expect this should need to be updated for other font modes.

@mstange
Copy link
Contributor Author

mstange commented Oct 22, 2017

I don't know the answers to these questions. I'm hoping Glenn does :)

@mstange
Copy link
Contributor Author

mstange commented Oct 22, 2017

I can add a new FontRenderMode value called Color and use that for Apple emojis. How does that sound, @glennw?

@glennw
Copy link
Member

glennw commented Oct 22, 2017

@mstange How about instead we have something like (with better naming perhaps):

enum BitmapColorMode {
    Color,
    Alpha,
    // None if we need in the future to ignore color *and* alpha??
}

enum FontRenderMode {
    ...
    Bitmap(BitmapColorMode)
}

This allows us to specify how to treat the color for a bitmap font, without needing a new font render mode.

@mstange
Copy link
Contributor Author

mstange commented Oct 23, 2017

@lsalzman had some concerns about this change. He is going to come up with a better fix.

@mstange mstange closed this Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.