Skip to content

Remove opacity multiplier field from BrushTip #310

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove opacity multiplier field from BrushTip #310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 6 additions & 41 deletions ink/brush/brush_family_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,15 +111,14 @@ TEST(BrushFamilyTest, StringifyWithNoId) {
absl::StatusOr<BrushFamily> family = BrushFamily::Create(
BrushTip{.scale = {3, 3},
.corner_rounding = 0,
.opacity_multiplier = 0.7,
.particle_gap_distance_scale = 0.1,
.particle_gap_duration = Duration32::Seconds(2)},
CreateTestPaint());
ASSERT_EQ(family.status(), absl::OkStatus());
EXPECT_EQ(absl::StrCat(*family),
"BrushFamily(coats=[BrushCoat{tip=BrushTip{scale=<3, 3>, "
"corner_rounding=0, opacity_multiplier=0.7, "
"particle_gap_distance_scale=0.1, particle_gap_duration=2s}, "
"corner_rounding=0, particle_gap_distance_scale=0.1, "
"particle_gap_duration=2s}, "
"paint_preferences={BrushPaint{texture_layers={TextureLayer{"
"client_texture_id=test-paint, mapping=kStamping, "
"origin=kStrokeSpaceOrigin, size_unit=kBrushSize, wrap_x=kRepeat, "
Expand All @@ -133,14 +132,13 @@ TEST(BrushFamilyTest, StringifyWithNoId) {
}

TEST(BrushFamilyTest, StringifyWithId) {
absl::StatusOr<BrushFamily> family = BrushFamily::Create(
BrushTip{
.scale = {3, 3}, .corner_rounding = 0, .opacity_multiplier = 0.7},
CreateTestPaint(), "big-square");
absl::StatusOr<BrushFamily> family =
BrushFamily::Create(BrushTip{.scale = {3, 3}, .corner_rounding = 0},
CreateTestPaint(), "big-square");
ASSERT_EQ(family.status(), absl::OkStatus());
EXPECT_EQ(absl::StrCat(*family),
"BrushFamily(coats=[BrushCoat{tip=BrushTip{scale=<3, 3>, "
"corner_rounding=0, opacity_multiplier=0.7}, "
"corner_rounding=0}, "
"paint_preferences={BrushPaint{texture_layers={TextureLayer{client_"
"texture_id=test-paint, mapping=kStamping, "
"origin=kStrokeSpaceOrigin, size_unit=kBrushSize, wrap_x=kRepeat, "
Expand Down Expand Up @@ -326,39 +324,6 @@ TEST(BrushFamilyTest, CreateWithInvalidTipRotation) {
}
}

TEST(BrushFamilyTest, CreateWithInvalidTipOpacityMultiplier) {
{
absl::Status status =
BrushFamily::Create({.opacity_multiplier = -kInfinity}, {}).status();
EXPECT_EQ(status.code(), kInvalidArgument);
EXPECT_THAT(status.message(), HasSubstr("opacity_multiplier"));
}
{
absl::Status status =
BrushFamily::Create({.opacity_multiplier = kInfinity}, {}).status();
EXPECT_EQ(status.code(), kInvalidArgument);
EXPECT_THAT(status.message(), HasSubstr("opacity_multiplier"));
}
{
absl::Status status =
BrushFamily::Create({.opacity_multiplier = kNan}, {}).status();
EXPECT_EQ(status.code(), kInvalidArgument);
EXPECT_THAT(status.message(), HasSubstr("opacity_multiplier"));
}
{
absl::Status status =
BrushFamily::Create({.opacity_multiplier = -1}, {}).status();
EXPECT_EQ(status.code(), kInvalidArgument);
EXPECT_THAT(status.message(), HasSubstr("opacity_multiplier"));
}
{
absl::Status status =
BrushFamily::Create({.opacity_multiplier = 5}, {}).status();
EXPECT_EQ(status.code(), kInvalidArgument);
EXPECT_THAT(status.message(), HasSubstr("opacity_multiplier"));
}
}

TEST(BrushFamilyTest, CreateWithInvalidTipSlant) {
{
absl::Status status =
Expand Down
5 changes: 2 additions & 3 deletions ink/brush/brush_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,7 @@ BrushFamily CreateTestFamily() {

TEST(BrushTest, Stringify) {
absl::StatusOr<BrushFamily> family = BrushFamily::Create(
BrushTip{
.scale = {3, 3}, .corner_rounding = 0, .opacity_multiplier = 0.7},
BrushTip{.scale = {3, 3}, .corner_rounding = 0},
{.texture_layers = {{.client_texture_id = std::string(kTestTextureId),
.mapping = BrushPaint::TextureMapping::kStamping,
.size_unit = BrushPaint::TextureSizeUnit::kBrushSize,
Expand All @@ -105,7 +104,7 @@ TEST(BrushTest, Stringify) {
"Brush(color=Color({0.000000, 0.000000, 1.000000, 1.000000}, sRGB), "
"size=3, epsilon=0.1, "
"family=BrushFamily(coats=[BrushCoat{tip=BrushTip{scale=<3, 3>, "
"corner_rounding=0, opacity_multiplier=0.7}, "
"corner_rounding=0}, "
"paint_preferences={BrushPaint{texture_layers={TextureLayer{client_"
"texture_id=test-texture, mapping=kStamping, "
"origin=kStrokeSpaceOrigin, size_unit=kBrushSize, wrap_x=kRepeat, "
Expand Down
10 changes: 0 additions & 10 deletions ink/brush/brush_tip.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,6 @@ absl::Status ValidateBrushTipTopLevel(const BrushTip& tip) {
return absl::InvalidArgumentError(absl::StrFormat(
"`BrushTip::rotation` must be finite. Got %v", tip.rotation));
}
if (!(tip.opacity_multiplier >= 0 && tip.opacity_multiplier <= 2)) {
return absl::InvalidArgumentError(
absl::StrFormat("`BrushTip::opacity_multiplier` must be a value in the "
"interval [0, 2]. Got %f",
tip.opacity_multiplier));
}
if (!std::isfinite(tip.particle_gap_distance_scale) ||
tip.particle_gap_distance_scale < 0) {
return absl::InvalidArgumentError(
Expand Down Expand Up @@ -107,10 +101,6 @@ std::string ToFormattedString(const BrushTip& tip) {
if (tip.rotation != Angle()) {
absl::StrAppend(&formatted, ", rotation=", tip.rotation);
}
if (tip.opacity_multiplier != 1.f) {
absl::StrAppend(&formatted,
", opacity_multiplier=", tip.opacity_multiplier);
}
if (tip.particle_gap_distance_scale != 0) {
absl::StrAppend(&formatted, ", particle_gap_distance_scale=",
tip.particle_gap_distance_scale);
Expand Down
6 changes: 0 additions & 6 deletions ink/brush/brush_tip.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,6 @@ struct BrushTip {
// Angle specifying the initial rotation of the tip shape after applying
// `scale`, `pinch`, and `slant`.
Angle rotation = Angle::Radians(0);
// Scales the opacity of the base brush color for this tip, independent of
// `brush_behavior`s. A possible example application is a highlighter brush.
//
// The multiplier must be in the range [0, 2] and the value ultimately applied
// can be modified by applicable `brush_behavior`s.
float opacity_multiplier = 1.f;
// Parameter controlling emission of particles as a function of distance
// traveled by the stroke inputs. The value must be finite and non-negative.
//
Expand Down
39 changes: 2 additions & 37 deletions ink/brush/brush_tip_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,13 @@ TEST(BrushTipTest, Stringify) {
.slant = Angle::Degrees(45),
.pinch = 0.75f,
.rotation = Angle::Degrees(90),
.opacity_multiplier = 0.7f,
}),
"BrushTip{scale=<0.5, 2>, corner_rounding=0.25, slant=0.25π, "
"pinch=0.75, rotation=0.5π, opacity_multiplier=0.7}");
"pinch=0.75, rotation=0.5π}");
EXPECT_EQ(
absl::StrCat(BrushTip{
.scale = {1.25f, 0.75f},
.corner_rounding = 0.f,
.opacity_multiplier = 0.7f,
.behaviors =
{
BrushBehavior{{
Expand All @@ -65,7 +63,7 @@ TEST(BrushTipTest, Stringify) {
}},
},
}),
"BrushTip{scale=<1.25, 0.75>, corner_rounding=0, opacity_multiplier=0.7, "
"BrushTip{scale=<1.25, 0.75>, corner_rounding=0, "
"behaviors={BrushBehavior{nodes={SourceNode{source=kTimeOfInputInMillis, "
"source_value_range={0, 250}}, TargetNode{target=kWidthMultiplier, "
"target_modifier_range={1.5, 2}}}}, "
Expand All @@ -81,7 +79,6 @@ TEST(BrushTipTest, EqualAndNotEqual) {
.slant = Angle::Degrees(45),
.pinch = 0.75f,
.rotation = Angle::Degrees(90),
.opacity_multiplier = 0.7f,
.particle_gap_distance_scale = 0.5f,
.particle_gap_duration = Duration32::Seconds(0.5),
.behaviors = {
Expand All @@ -105,7 +102,6 @@ TEST(BrushTipTest, EqualAndNotEqual) {
.slant = Angle::Degrees(45),
.pinch = 0.75f,
.rotation = Angle::Degrees(90),
.opacity_multiplier = 0.7f,
.particle_gap_distance_scale = 0.5f,
.particle_gap_duration = Duration32::Seconds(0.5),
.behaviors = {
Expand All @@ -127,7 +123,6 @@ TEST(BrushTipTest, EqualAndNotEqual) {
.slant = Angle::Degrees(45),
.pinch = 0.75f,
.rotation = Angle::Degrees(90),
.opacity_multiplier = 0.7f,
.particle_gap_distance_scale = 0.5f,
.particle_gap_duration = Duration32::Seconds(0.5),
.behaviors = {BrushBehavior{{
Expand All @@ -148,7 +143,6 @@ TEST(BrushTipTest, EqualAndNotEqual) {
.slant = Angle::Degrees(45),
.pinch = 0.75f,
.rotation = Angle::Degrees(90),
.opacity_multiplier = 0.7f,
.particle_gap_distance_scale = 0.5f,
.particle_gap_duration = Duration32::Seconds(0.5),
.behaviors = {
Expand All @@ -171,7 +165,6 @@ TEST(BrushTipTest, EqualAndNotEqual) {
.slant = Angle::Degrees(33), // Modified
.pinch = 0.75f,
.rotation = Angle::Degrees(90),
.opacity_multiplier = 0.7f,
.particle_gap_distance_scale = 0.5f,
.particle_gap_duration = Duration32::Seconds(0.5),
.behaviors = {
Expand All @@ -194,7 +187,6 @@ TEST(BrushTipTest, EqualAndNotEqual) {
.slant = Angle::Degrees(45),
.pinch = 0.88f, // Modified
.rotation = Angle::Degrees(90),
.opacity_multiplier = 0.7f,
.particle_gap_distance_scale = 0.5f,
.particle_gap_duration = Duration32::Seconds(0.5),
.behaviors = {
Expand All @@ -217,7 +209,6 @@ TEST(BrushTipTest, EqualAndNotEqual) {
.slant = Angle::Degrees(45),
.pinch = 0.75f,
.rotation = Angle::Degrees(22), // Modified
.opacity_multiplier = 0.7f,
.particle_gap_distance_scale = 0.5f,
.particle_gap_duration = Duration32::Seconds(0.5),
.behaviors = {
Expand All @@ -240,30 +231,6 @@ TEST(BrushTipTest, EqualAndNotEqual) {
.slant = Angle::Degrees(45),
.pinch = 0.75f,
.rotation = Angle::Degrees(90),
.opacity_multiplier = 0.9f, // Modified
.particle_gap_distance_scale = 0.5f,
.particle_gap_duration = Duration32::Seconds(0.5),
.behaviors = {
BrushBehavior{{
BrushBehavior::SourceNode{
.source = BrushBehavior::Source::kTimeOfInputInMillis,
.source_value_range = {0, 250},
},
BrushBehavior::TargetNode{
.target = BrushBehavior::Target::kWidthMultiplier,
.target_modifier_range = {1.5, 2},
},
}},
}}));
EXPECT_NE(
brush_tip,
BrushTip(
{.scale = {1.25f, 0.75f},
.corner_rounding = 0.25f,
.slant = Angle::Degrees(45),
.pinch = 0.75f,
.rotation = Angle::Degrees(90),
.opacity_multiplier = 0.7f,
.particle_gap_distance_scale = 0, // Modified
.particle_gap_duration = Duration32::Seconds(0.5),
.behaviors = {
Expand All @@ -286,7 +253,6 @@ TEST(BrushTipTest, EqualAndNotEqual) {
.slant = Angle::Degrees(45),
.pinch = 0.75f,
.rotation = Angle::Degrees(90),
.opacity_multiplier = 0.7f,
.particle_gap_distance_scale = 0.5f,
.particle_gap_duration = Duration32::Zero(), // Modified
.behaviors = {
Expand All @@ -309,7 +275,6 @@ TEST(BrushTipTest, EqualAndNotEqual) {
.slant = Angle::Degrees(45),
.pinch = 0.75f,
.rotation = Angle::Degrees(90),
.opacity_multiplier = 0.7f,
.particle_gap_distance_scale = 0.5f,
.particle_gap_duration = Duration32::Seconds(0.5),
// Modified:
Expand Down
5 changes: 2 additions & 3 deletions ink/brush/fuzz_domains.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,9 +713,8 @@ Domain<BrushTip> ValidBrushTip(DomainVariant variant) {
Filter([](Vec scale) { return scale != Vec(); },
StructOf<Vec>(FiniteNonNegativeFloat(), FiniteNonNegativeFloat())),
InRange<float>(0.f, 1.f), AngleInRange(-kQuarterTurn, kQuarterTurn),
InRange<float>(0.f, 1.f), FiniteAngle(), InRange<float>(0.f, 2.f),
FiniteNonNegativeFloat(), FiniteNonNegativeDuration32(),
VectorOf(ValidBrushBehavior(variant)));
InRange<float>(0.f, 1.f), FiniteAngle(), FiniteNonNegativeFloat(),
FiniteNonNegativeDuration32(), VectorOf(ValidBrushBehavior(variant)));
}

} // namespace
Expand Down
10 changes: 2 additions & 8 deletions ink/brush/internal/jni/brush_tip_jni.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ extern "C" {
JNI_METHOD(brush, BrushTipNative, jlong, create)
(JNIEnv* env, jobject thiz, jfloat scale_x, jfloat scale_y,
jfloat corner_rounding, jfloat slant_radians, jfloat pinch,
jfloat rotation_radians, jfloat opacity_multiplier,
jfloat particle_gap_distance_scale, jlong particle_gap_duration_millis,
jfloat rotation_radians, jfloat particle_gap_distance_scale,
jlong particle_gap_duration_millis,
jlongArray behavior_native_pointers_array) {
std::vector<ink::BrushBehavior> behaviors;
const jsize num_behaviors =
Expand All @@ -72,7 +72,6 @@ JNI_METHOD(brush, BrushTipNative, jlong, create)
.slant = ink::Angle::Radians(slant_radians),
.pinch = pinch,
.rotation = ink::Angle::Radians(rotation_radians),
.opacity_multiplier = opacity_multiplier,
.particle_gap_distance_scale = particle_gap_distance_scale,
.particle_gap_duration = Duration32::Millis(particle_gap_duration_millis),
.behaviors = behaviors};
Expand Down Expand Up @@ -118,11 +117,6 @@ JNI_METHOD(brush, BrushTipNative, jfloat, getRotationRadians)
return CastToBrushTip(native_pointer).rotation.ValueInRadians();
}

JNI_METHOD(brush, BrushTipNative, jfloat, getOpacityMultiplier)
(JNIEnv* env, jobject thiz, jlong native_pointer) {
return CastToBrushTip(native_pointer).opacity_multiplier;
}

JNI_METHOD(brush, BrushTipNative, jfloat, getParticleGapDistanceScale)
(JNIEnv* env, jobject thiz, jlong native_pointer) {
return CastToBrushTip(native_pointer).particle_gap_distance_scale;
Expand Down
2 changes: 0 additions & 2 deletions ink/brush/type_matchers.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,8 +285,6 @@ MATCHER_P(BrushTipEqMatcher, expected,
Field("slant", &BrushTip::slant, AngleEq(expected.slant)),
Field("pinch", &BrushTip::pinch, FloatEq(expected.pinch)),
Field("rotation", &BrushTip::rotation, AngleEq(expected.rotation)),
Field("opacity_multiplier", &BrushTip::opacity_multiplier,
FloatEq(expected.opacity_multiplier)),
Field("particle_gap_distance_scale",
&BrushTip::particle_gap_distance_scale,
FloatEq(expected.particle_gap_distance_scale)),
Expand Down
16 changes: 6 additions & 10 deletions ink/rendering/skia/native/internal/path_drawable.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,8 @@ void SetPaintDefaultsForPath(SkPaint& paint) {
PathDrawable::PathDrawable(
const MutableMesh& mesh,
absl::Span<const absl::Span<const uint32_t>> index_outlines,
absl::Span<const ColorFunction> color_functions, float opacity_multiplier)
: color_functions_(color_functions.begin(), color_functions.end()),
opacity_multiplier_(opacity_multiplier) {
absl::Span<const ColorFunction> color_functions)
: color_functions_(color_functions.begin(), color_functions.end()) {
for (absl::Span<const uint32_t> indices : index_outlines) {
if (indices.empty()) continue;

Expand All @@ -106,10 +105,8 @@ PathDrawable::PathDrawable(

PathDrawable::PathDrawable(const PartitionedMesh& shape,
uint32_t render_group_index,
absl::Span<const ColorFunction> color_functions,
float opacity_multiplier)
: color_functions_(color_functions.begin(), color_functions.end()),
opacity_multiplier_(opacity_multiplier) {
absl::Span<const ColorFunction> color_functions)
: color_functions_(color_functions.begin(), color_functions.end()) {
absl::Span<const Mesh> mesh_group =
shape.RenderGroupMeshes(render_group_index);
for (uint32_t i = 0; i < shape.OutlineCount(render_group_index); ++i) {
Expand All @@ -127,9 +124,8 @@ void PathDrawable::SetBrushColor(const Color& brush_color) {
.InColorSpace(ColorSpace::kSrgb)
.AsFloat(Color::Format::kLinear);
sk_sp<SkColorSpace> srgb_linear = SkColorSpace::MakeSRGBLinear();
paint_.setColor(
{.fR = c.r, .fG = c.g, .fB = c.b, .fA = c.a * opacity_multiplier_},
srgb_linear.get());
paint_.setColor({.fR = c.r, .fG = c.g, .fB = c.b, .fA = c.a},
srgb_linear.get());
}

void PathDrawable::SetImageFilter(sk_sp<SkImageFilter> image_filter) {
Expand Down
12 changes: 4 additions & 8 deletions ink/rendering/skia/native/internal/path_drawable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,11 @@ class PathDrawable {
// function will need to change to accept a `span<const MutableMesh>`.
PathDrawable(const MutableMesh& mesh,
absl::Span<const absl::Span<const uint32_t>> index_outlines,
absl::Span<const ColorFunction> color_functions,
float opacity_multiplier);
absl::Span<const ColorFunction> color_functions);

// Constructs the drawable from one render group of a `PartitionedMesh`.
PathDrawable(const PartitionedMesh& shape, uint32_t render_group_index,
absl::Span<const ColorFunction> color_functions,
float opacity_multiplier);
absl::Span<const ColorFunction> color_functions);

PathDrawable() = default;
PathDrawable(const PathDrawable&) = default;
Expand All @@ -60,9 +58,8 @@ class PathDrawable {
~PathDrawable() = default;

// Sets the brush color to be used for this drawable. The passed-in
// `brush_color` will be transformed by the `color_functions` and
// `opacity_multiplier` passed in during construction before being applied to
// the underlying `SkPaint`.
// `brush_color` will be transformed by the `color_functions` passed in during
// construction before being applied to the underlying `SkPaint`.
void SetBrushColor(const Color& brush_color);

void SetImageFilter(sk_sp<SkImageFilter> image_filter);
Expand All @@ -73,7 +70,6 @@ class PathDrawable {
absl::InlinedVector<SkPath, 1> paths_;
SkPaint paint_;
absl::InlinedVector<ColorFunction, 1> color_functions_;
float opacity_multiplier_;
};

} // namespace ink::skia_native_internal
Expand Down
Loading
Loading