-
Notifications
You must be signed in to change notification settings - Fork 101
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
avoid using grayscale base when mixing richer colors #2408
avoid using grayscale base when mixing richer colors #2408
Conversation
lib/LaTeXML/Common/Color.pm
Outdated
# have components discarded. | ||
if ($base_model eq 'gray') { | ||
my $enriched_base = $self->convert($mix_model); | ||
return $enriched_base->mix($color, $fraction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting; looks like you've likely got the right idea. I'm wondering out-loud whether it might be slightly less convoluted than a return from the middle to start with my $base = $self;
and then if gray, do $base = $self->convert($mix_model);
, and do the last few lines with $base
instead of $self
? That would seem to make more transparent that you're really just re-expressing self in a different color model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, the extra recursion is just needless performance tax here. Now adjusted.
lib/LaTeXML/Common/Color.pm
Outdated
my @a = $self->components; | ||
my @b = $color->components; | ||
my @b = $other->components; | ||
return $self->new(map { $a[$_] + $b[$_] } 0 .. $#a); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't the last 2 $self
be $base
? (like for mix
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duh. How did that slip happen? One quote I overheard this morning:
"If it isn't tested, it is broken."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for catching that and avoiding a very annoying encounter+report+debug cascade later on. Fixed.
1939322
to
64349cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
* avoid using grayscale base when mixing richer colors * avoid needless recursion in color mixing/adding
Fixes #2407
The problem was that when a base model is grayscale, a
mix
operation withrgb
would remain in the base model - i.e. remain in grayscale. So whenmix
oradd
executes, the "richer" color model ought to be used.Suggestions welcome if my code should be structured better, I almost made a new
sub
to hold the model-switching logic. Are there more places that could benefit from this treatment?