Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] expose reference to tessellator instead of shared_ptr. #56244

Merged
auto-submit merged 1 commit into from
Oct 31, 2024
Merged

[Impeller] expose reference to tessellator instead of shared_ptr. #56244

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
4 changes: 2 additions & 2 deletions impeller/entity/contents/content_context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,8 +543,8 @@ fml::StatusOr<RenderTarget> ContentContext::MakeSubpass(
return subpass_target;
}

std::shared_ptr<Tessellator> ContentContext::GetTessellator() const {
return tessellator_;
Tessellator& ContentContext::GetTessellator() const {
Copy link
Member

@gaaclarke gaaclarke Oct 30, 2024

Choose a reason for hiding this comment

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

This should be const Tessellator& ContentContext::GetTessellator() const or Tessellator& ContentContext::GetTessellator()

https://google.github.io/styleguide/cppguide.html#Use_of_const

Copy link
Contributor Author

@jonahwilliams jonahwilliams Oct 30, 2024

Choose a reason for hiding this comment

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

tessellator methods are non const because it stores an allocation arena we use.

Copy link
Member

@gaaclarke gaaclarke Oct 30, 2024

Choose a reason for hiding this comment

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

So Tessellator& ContentContext::GetTessellator()?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Oct 30, 2024

Choose a reason for hiding this comment

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

Yeah?

Copy link
Member

@gaaclarke gaaclarke Oct 31, 2024

Choose a reason for hiding this comment

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

That's not what's there, you have a const method returning a non-const ref =)

Copy link
Member

@gaaclarke gaaclarke Oct 31, 2024

Choose a reason for hiding this comment

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

Declare methods to be const unless they alter the logical state of the object (or enable the user to modify that state, e.g., by returning a non-const reference, but that's rare), or they can't safely be invoked concurrently.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Oct 31, 2024

Choose a reason for hiding this comment

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

I don't think that is what that means. The ContextVK is const, the tessellator is not. same with the host buffer?

Copy link
Member

@gaaclarke gaaclarke Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Constness should be transitive where if X is a aggregate of Y and Z and you have a const X, you should only be able to get a const Y& and const Z&.

There are already instances where this isn't the case here and admittedly Context is an odd grab bag of state. I was suggesting it if it's easy to do, otherwise this is fine since it doesn't regress what was already there.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Oct 31, 2024

Choose a reason for hiding this comment

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

If that is an important policy to enforce, then it really needs to enforced via lints or the compiler.

return *tessellator_;
}

std::shared_ptr<Context> ContentContext::GetContext() const {
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/contents/content_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ class ContentContext {

bool IsValid() const;

std::shared_ptr<Tessellator> GetTessellator() const;
Tessellator& GetTessellator() const;

std::shared_ptr<Pipeline<PipelineDescriptor>> GetFastGradientPipeline(
ContentContextOptions opts) const {
Expand Down
6 changes: 2 additions & 4 deletions impeller/entity/geometry/circle_geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,10 @@ GeometryResult CircleGeometry::GetPositionBuffer(const ContentContext& renderer,
transform, stroke_width_,
pass.GetSampleCount() == SampleCount::kCount4);

const std::shared_ptr<Tessellator>& tessellator = renderer.GetTessellator();

// We call the StrokedCircle method which will simplify to a
// FilledCircleGenerator if the inner_radius is <= 0.
auto generator =
tessellator->StrokedCircle(transform, center_, radius_, half_width);
auto generator = renderer.GetTessellator().StrokedCircle(transform, center_,
radius_, half_width);

return ComputePositionGeometry(renderer, generator, entity, pass);
}
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/geometry/ellipse_geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ GeometryResult EllipseGeometry::GetPositionBuffer(
RenderPass& pass) const {
return ComputePositionGeometry(
renderer,
renderer.GetTessellator()->FilledEllipse(entity.GetTransform(), bounds_),
renderer.GetTessellator().FilledEllipse(entity.GetTransform(), bounds_),
entity, pass);
}

Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/geometry/fill_path_geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ GeometryResult FillPathGeometry::GetPositionBuffer(
};
}

VertexBuffer vertex_buffer = renderer.GetTessellator()->TessellateConvex(
VertexBuffer vertex_buffer = renderer.GetTessellator().TessellateConvex(
path_, host_buffer, entity.GetTransform().GetMaxBasisLengthXY());

return GeometryResult{
Expand Down
4 changes: 2 additions & 2 deletions impeller/entity/geometry/line_geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ GeometryResult LineGeometry::GetPositionBuffer(const ContentContext& renderer,
transform, width_, pass.GetSampleCount() == SampleCount::kCount4);

if (cap_ == Cap::kRound) {
std::shared_ptr<Tessellator> tessellator = renderer.GetTessellator();
auto generator = tessellator->RoundCapLine(transform, p0_, p1_, radius);
auto generator =
renderer.GetTessellator().RoundCapLine(transform, p0_, p1_, radius);
return ComputePositionGeometry(renderer, generator, entity, pass);
}

Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/geometry/point_field_geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ GeometryResult PointFieldGeometry::GetPositionBuffer(
// Get triangulation relative to {0, 0} so we can translate it to each
// point in turn.
Tessellator::EllipticalVertexGenerator generator =
renderer.GetTessellator()->FilledCircle(transform, {}, radius);
renderer.GetTessellator().FilledCircle(transform, {}, radius);
FML_DCHECK(generator.GetTriangleType() == PrimitiveType::kTriangleStrip);

std::vector<Point> circle_vertices;
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/geometry/round_rect_geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ GeometryResult RoundRectGeometry::GetPositionBuffer(
const Entity& entity,
RenderPass& pass) const {
return ComputePositionGeometry(renderer,
renderer.GetTessellator()->FilledRoundRect(
renderer.GetTessellator().FilledRoundRect(
entity.GetTransform(), bounds_, radii_),
entity, pass);
}
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/geometry/stroke_path_geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,7 @@ GeometryResult StrokePathGeometry::GetPositionBuffer(
auto scale = entity.GetTransform().GetMaxBasisLengthXY();

PositionWriter position_writer;
auto polyline = renderer.GetTessellator()->CreateTempPolyline(path_, scale);
auto polyline = renderer.GetTessellator().CreateTempPolyline(path_, scale);
CreateSolidStrokeVertices(position_writer, polyline, stroke_width,
miter_limit_ * stroke_width_ * 0.5f,
GetJoinProc<PositionWriter>(stroke_join_),
Expand Down