Skip to content

Conversation

@SimonDanisch
Copy link
Member

No description provided.

Copy link
Member

@jkrumbiegel jkrumbiegel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is one of these places in the code base where it's difficult to know what type color etc could have, and therefore how the code will execute. also, I guess we recompile this expensive looking function for a color given as a Symbol vs. as an RGBf etc. I would like clearer logic, or does that tie into the attribute refactor idea?

[color for x in charinfos],
[strokecolor for x in charinfos],
[strokewidth for x in charinfos],
per_char_info(rotation, charinfos),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm neither what I had before nor this seems ideal. there is already a function called attribute_per_char at the top, this is currently used for font and textsize, but similar logic could apply to color. anyway, it feels weird to do one expansion before the glyph_collection function and one here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I somehow didnt find that function... So I guess I should just use that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this uses attribute_per_char now!

@SimonDanisch
Copy link
Member Author

I would like clearer logic, or does that tie into the attribute refactor idea?

To separate the concerns a bit, I would leave this for the attribute refactor PR, as long as this PR doesn't introduce any regressions

@SimonDanisch SimonDanisch merged commit cc5a0a8 into master Jan 18, 2022
@SimonDanisch SimonDanisch deleted the sd/color-per-glyph branch January 18, 2022 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants