[scenic] Remove NodeParts
Delete all scenic code related to node parts. All non-scenic
calls to AddParts have been removed in other CLs. Existing API
in commands.fidl union remains as to not break the ABI contract,
however it is now a no-op and will result in a session error
if an attempt is made to call it.
Testing - Added new unit test to scenic_unittest.cc to make sure
the scenic session is closed when AddParts is called.
Change-Id: Icb9332b30df4d34895c3c442791ffb441f9080c6
diff --git a/garnet/lib/ui/gfx/engine/engine_renderer_visitor.cc b/garnet/lib/ui/gfx/engine/engine_renderer_visitor.cc
index 74caeb4..638737d 100644
--- a/garnet/lib/ui/gfx/engine/engine_renderer_visitor.cc
+++ b/garnet/lib/ui/gfx/engine/engine_renderer_visitor.cc
@@ -116,9 +116,8 @@
void EngineRendererVisitor::Visit(Layer* r) { FXL_DCHECK(false); }
void EngineRendererVisitor::Visit(ShapeNode* r) {
- // We don't need to call |VisitNode| because shape nodes don't have
- // children or parts.
- FXL_DCHECK(r->children().empty() && r->parts().empty());
+ // We don't need to call |VisitNode| because shape nodes don't have children.
+ FXL_DCHECK(r->children().empty());
auto& shape = r->shape();
auto& material = r->material();
diff --git a/garnet/lib/ui/gfx/engine/gfx_command_applier.cc b/garnet/lib/ui/gfx/engine/gfx_command_applier.cc
index 85dcea3..5443176 100644
--- a/garnet/lib/ui/gfx/engine/gfx_command_applier.cc
+++ b/garnet/lib/ui/gfx/engine/gfx_command_applier.cc
@@ -384,12 +384,9 @@
}
bool GfxCommandApplier::ApplyAddPartCmd(Session* session, fuchsia::ui::gfx::AddPartCmd command) {
- // Find the parent and part nodes.
- if (auto parent_node = session->resources()->FindResource<Node>(command.node_id)) {
- if (auto part_node = session->resources()->FindResource<Node>(command.part_id)) {
- return parent_node->AddPart(std::move(part_node), session->error_reporter());
- }
- }
+ // This is now a no-op.
+ FXL_LOG(INFO) << "AddPart is illegal now.";
+ session->error_reporter()->ERROR() << "AddPartCmd is now a no-op. Do not use.";
return false;
}
diff --git a/garnet/lib/ui/gfx/engine/hit_tester.cc b/garnet/lib/ui/gfx/engine/hit_tester.cc
index 25b87e0..36939fe 100644
--- a/garnet/lib/ui/gfx/engine/hit_tester.cc
+++ b/garnet/lib/ui/gfx/engine/hit_tester.cc
@@ -154,7 +154,7 @@
}
void HitTester::AccumulateHitsInner(Node* node) {
- if (node->clip_to_self() && !IsRayWithinPartsInner(node, ray_info_->ray, *intersection_info_))
+ if (node->clip_to_self())
return;
Node::IntersectionInfo* outer_intersection = intersection_info_;
@@ -173,48 +173,5 @@
intersection_info_ = outer_intersection;
}
-// TODO(SCN-1493): This is only used for testing against "parts".
-bool HitTester::IsRayWithinPartsInner(const Node* node, const escher::ray4& ray,
- const Node::IntersectionInfo& intersection) {
- return ForEachPartFrontToBackUntilTrue(*node, [&ray, &intersection](Node* node) {
- return IsRayWithinClippedContentOuter(node, ray, intersection);
- });
-}
-
-// TODO(SCN-1493): This is only used for testing against "parts".
-bool HitTester::IsRayWithinClippedContentOuter(const Node* node, const escher::ray4& ray,
- const Node::IntersectionInfo& intersection) {
- if (node->transform().IsIdentity()) {
- return IsRayWithinClippedContentInner(node, ray, intersection);
- }
-
- auto inverse_transform = glm::inverse(static_cast<glm::mat4>(node->transform()));
- escher::ray4 local_ray = inverse_transform * ray;
-
- Node::IntersectionInfo local_intersection =
- GetTransformedIntersection(intersection, ray, local_ray, inverse_transform);
-
- return IsRayWithinClippedContentInner(node, local_ray, local_intersection);
-}
-
-// TODO(SCN-1493): This is only used for testing against "parts".
-bool HitTester::IsRayWithinClippedContentInner(const Node* node, const escher::ray4& ray,
- const Node::IntersectionInfo& intersection) {
- Node::IntersectionInfo new_intersection = node->GetIntersection(ray, intersection);
- if (new_intersection.did_hit)
- return true;
-
- // TODO(SCN-1493): Get rid of node "parts".
- if (IsRayWithinPartsInner(node, ray, intersection))
- return true;
-
- if (node->clip_to_self())
- return false;
-
- return ForEachChildAndImportFrontToBackUntilTrue(*node, [&ray, &intersection](Node* node) {
- return IsRayWithinClippedContentOuter(node, ray, intersection);
- });
-}
-
} // namespace gfx
} // namespace scenic_impl
diff --git a/garnet/lib/ui/gfx/engine/hit_tester.h b/garnet/lib/ui/gfx/engine/hit_tester.h
index fdce0cb..136b2ed 100644
--- a/garnet/lib/ui/gfx/engine/hit_tester.h
+++ b/garnet/lib/ui/gfx/engine/hit_tester.h
@@ -74,14 +74,6 @@
// |ray_info_| must be in the node's local coordinate system.
void AccumulateHitsInner(Node* node);
- // Returns true if the ray passes through the node's parts.
- // |ray| must be in the node's local coordinate system.
- //
- // TODO(SCN-1493): Get rid of node "parts". This function only runs on
- // node parts.
- static bool IsRayWithinPartsInner(const Node* node, const escher::ray4& ray,
- const Node::IntersectionInfo& intersection);
-
// Returns true if the ray passes through the node's clipped content.
// |ray| must be in the parent's local coordinate system.
//
diff --git a/garnet/lib/ui/gfx/resources/dump_visitor.cc b/garnet/lib/ui/gfx/resources/dump_visitor.cc
index a21b675..e56b2f0 100644
--- a/garnet/lib/ui/gfx/resources/dump_visitor.cc
+++ b/garnet/lib/ui/gfx/resources/dump_visitor.cc
@@ -182,13 +182,6 @@
} else {
WriteProperty("transform") << r->transform();
}
- if (!r->parts().empty()) {
- BeginSection("parts");
- for (auto& part : r->parts()) {
- part->Accept(this);
- }
- EndSection();
- }
if (!r->children().empty()) {
BeginSection("children");
for (auto& child : r->children()) {
diff --git a/garnet/lib/ui/gfx/resources/nodes/node.cc b/garnet/lib/ui/gfx/resources/nodes/node.cc
index 7e0da57..5084e23 100644
--- a/garnet/lib/ui/gfx/resources/nodes/node.cc
+++ b/garnet/lib/ui/gfx/resources/nodes/node.cc
@@ -22,8 +22,7 @@
constexpr ResourceTypeFlags kHasChildren = ResourceType::kEntityNode | ResourceType::kOpacityNode |
ResourceType::kScene | ResourceType::kView;
-constexpr ResourceTypeFlags kHasParts =
- ResourceType::kEntityNode | ResourceType::kOpacityNode | ResourceType::kClipNode;
+
constexpr ResourceTypeFlags kHasTransform = ResourceType::kClipNode | ResourceType::kEntityNode |
ResourceType::kOpacityNode | ResourceType::kScene |
ResourceType::kShapeNode | ResourceType::kViewHolder;
@@ -84,24 +83,6 @@
return true;
}
-bool Node::AddPart(NodePtr part_node, ErrorReporter* error_reporter) {
- if (!(type_flags() & kHasParts)) {
- error_reporter->ERROR() << "scenic::gfx::Node::AddPart(): node of type " << type_name()
- << " cannot have parts.";
- return false;
- }
-
- if (part_node->parent_relation_ == ParentRelation::kPart && part_node->parent_ == this) {
- return true; // no change
- }
-
- // Detach and re-attach Node to us.
- part_node->Detach(error_reporter);
- part_node->SetParent(this, ParentRelation::kPart);
- parts_.push_back(std::move(part_node));
- return true;
-}
-
void Node::SetParent(Node* parent, ParentRelation relation) {
FXL_DCHECK(parent_ == nullptr);
// A Scene node should always be a root node, and never a child.
@@ -118,9 +99,6 @@
case ParentRelation::kChild:
parent_->EraseChild(this);
break;
- case ParentRelation::kPart:
- parent_->ErasePart(this);
- break;
case ParentRelation::kImportDelegate:
error_reporter->ERROR() << "An imported node cannot be detached.";
return false;
@@ -378,13 +356,6 @@
children_.erase(it);
}
-void Node::ErasePart(Node* part) {
- auto it = std::find_if(parts_.begin(), parts_.end(),
- [part](const NodePtr& ptr) { return part == ptr.get(); });
- FXL_DCHECK(it != parts_.end());
- parts_.erase(it);
-}
-
void Node::DetachInternal() {
parent_relation_ = ParentRelation::kNone;
parent_ = nullptr;
diff --git a/garnet/lib/ui/gfx/resources/nodes/node.h b/garnet/lib/ui/gfx/resources/nodes/node.h
index 4d4d8730..9ef4f50 100644
--- a/garnet/lib/ui/gfx/resources/nodes/node.h
+++ b/garnet/lib/ui/gfx/resources/nodes/node.h
@@ -54,7 +54,6 @@
virtual ~Node() override;
bool AddChild(NodePtr child_node, ErrorReporter* error_reporter);
- bool AddPart(NodePtr part_node, ErrorReporter* error_reporter);
bool DetachChildren(ErrorReporter* error_reporter);
bool SetTagValue(uint32_t tag_value);
@@ -101,8 +100,6 @@
const std::vector<NodePtr>& children() const { return children_; }
- const std::vector<NodePtr>& parts() const { return parts_; }
-
bool SetEventMask(uint32_t event_mask) override;
void AddImport(Import* import, ErrorReporter* error_reporter) override;
@@ -140,7 +137,7 @@
private:
// Describes the manner in which a node is related to its parent.
- enum class ParentRelation { kNone, kChild, kPart, kImportDelegate };
+ enum class ParentRelation { kNone, kChild, kImportDelegate };
// Identifies a specific spatial property.
enum NodeProperty { kTranslation, kScale, kRotation, kAnchor };
@@ -167,7 +164,6 @@
ParentRelation parent_relation_ = ParentRelation::kNone;
// TODO(SCN-1299) Split out child behavior into ContainerNode class.
std::vector<NodePtr> children_;
- std::vector<NodePtr> parts_;
std::unordered_map<NodeProperty, std::unique_ptr<VariableBinding>> bound_variables_;
diff --git a/garnet/lib/ui/gfx/resources/nodes/traversal.h b/garnet/lib/ui/gfx/resources/nodes/traversal.h
index 228bfab..0fafcfb 100644
--- a/garnet/lib/ui/gfx/resources/nodes/traversal.h
+++ b/garnet/lib/ui/gfx/resources/nodes/traversal.h
@@ -24,15 +24,6 @@
//
// The functor's signature must be |void(Node* node)|.
//
-
-template <typename Callable>
-void ForEachPartFrontToBack(const Node& node, const Callable& func) {
- // Process most recently added parts first.
- for (auto it = node.parts().rbegin(); it != node.parts().rend(); ++it) {
- func(it->get());
- }
-}
-
template <typename Callable>
void ForEachChildFrontToBack(const Node& node, const Callable& func) {
// Process most recently added children first.
@@ -58,7 +49,6 @@
template <typename Callable>
void ForEachDirectDescendantFrontToBack(const Node& node, const Callable& func) {
ForEachChildAndImportFrontToBack(node, func);
- ForEachPartFrontToBack(node, func);
}
//
@@ -66,17 +56,6 @@
//
// The functor's signature must be |bool(const Node* node)|.
//
-
-template <typename Callable>
-bool ForEachPartFrontToBackUntilTrue(const Node& node, const Callable& func) {
- // Process most recently added parts first.
- for (auto it = node.parts().rbegin(); it != node.parts().rend(); ++it) {
- if (func(it->get()))
- return true;
- }
- return false;
-}
-
template <typename Callable>
bool ForEachChildFrontToBackUntilTrue(const Node& node, const Callable& func) {
// Process most recently added children first.
diff --git a/garnet/lib/ui/gfx/resources/snapshot/snapshotter.cc b/garnet/lib/ui/gfx/resources/snapshot/snapshotter.cc
index 5b46ec2..ff8a4a5 100644
--- a/garnet/lib/ui/gfx/resources/snapshot/snapshotter.cc
+++ b/garnet/lib/ui/gfx/resources/snapshot/snapshotter.cc
@@ -240,12 +240,6 @@
}
// Children.
- for (auto& part : r->parts()) {
- // Set current node to this node during children traversal.
- current_node_serializer_ = node_serializer;
- part->Accept(this);
- }
-
for (auto& child : r->children()) {
// Set current node to this node during children traversal.
current_node_serializer_ = node_serializer;
diff --git a/garnet/lib/ui/gfx/tests/session_unittest.cc b/garnet/lib/ui/gfx/tests/session_unittest.cc
index 0783406..47e4a25 100644
--- a/garnet/lib/ui/gfx/tests/session_unittest.cc
+++ b/garnet/lib/ui/gfx/tests/session_unittest.cc
@@ -16,13 +16,23 @@
namespace gfx {
namespace test {
+
+TEST_F(SessionTest, AddPart_ShouldReturnFalse) {
+ fuchsia::ui::gfx::AddPartCmd add_part_command;
+ add_part_command.node_id = 0;
+ add_part_command.part_id = 1;
+ fuchsia::ui::gfx::Command command;
+ command.set_add_part(std::move(add_part_command));
+ EXPECT_FALSE(Apply(std::move(command)));
+}
+
TEST_F(SessionTest, ScheduleUpdateOutOfOrder) {
EXPECT_TRUE(session()->ScheduleUpdate(
- /*presentation*/ zx::time(1), std::vector<::fuchsia::ui::gfx::Command>(), std::vector<zx::event>(),
- std::vector<zx::event>(), [](auto) {}));
+ /*presentation*/ zx::time(1), std::vector<::fuchsia::ui::gfx::Command>(),
+ std::vector<zx::event>(), std::vector<zx::event>(), [](auto) {}));
EXPECT_FALSE(session()->ScheduleUpdate(
- /*presentation*/ zx::time(0), std::vector<::fuchsia::ui::gfx::Command>(), std::vector<zx::event>(),
- std::vector<zx::event>(), [](auto) {}));
+ /*presentation*/ zx::time(0), std::vector<::fuchsia::ui::gfx::Command>(),
+ std::vector<zx::event>(), std::vector<zx::event>(), [](auto) {}));
ExpectLastReportedError(
"scenic_impl::gfx::Session: Present called with out-of-order "
"presentation "
@@ -32,11 +42,11 @@
TEST_F(SessionTest, ScheduleUpdateInOrder) {
EXPECT_TRUE(session()->ScheduleUpdate(
- /*presentation*/ zx::time(1), std::vector<::fuchsia::ui::gfx::Command>(), std::vector<zx::event>(),
- std::vector<zx::event>(), [](auto) {}));
+ /*presentation*/ zx::time(1), std::vector<::fuchsia::ui::gfx::Command>(),
+ std::vector<zx::event>(), std::vector<zx::event>(), [](auto) {}));
EXPECT_TRUE(session()->ScheduleUpdate(
- /*presentation*/ zx::time(1), std::vector<::fuchsia::ui::gfx::Command>(), std::vector<zx::event>(),
- std::vector<zx::event>(), [](auto) {}));
+ /*presentation*/ zx::time(1), std::vector<::fuchsia::ui::gfx::Command>(),
+ std::vector<zx::event>(), std::vector<zx::event>(), [](auto) {}));
ExpectLastReportedError(nullptr);
}
@@ -67,7 +77,6 @@
EXPECT_TRUE(Apply(scenic::NewCreateShapeNodeCmd(3)));
EXPECT_TRUE(Apply(scenic::NewCreateShapeNodeCmd(4)));
EXPECT_TRUE(Apply(scenic::NewAddChildCmd(1, 2)));
- EXPECT_TRUE(Apply(scenic::NewAddPartCmd(1, 3)));
EXPECT_EQ(4U, session()->GetTotalResourceCount());
EXPECT_EQ(4U, session()->GetMappedResourceCount());
@@ -76,7 +85,7 @@
EXPECT_TRUE(Apply(scenic::NewReleaseResourceCmd(2)));
EXPECT_TRUE(Apply(scenic::NewReleaseResourceCmd(3)));
EXPECT_TRUE(Apply(scenic::NewReleaseResourceCmd(4)));
- EXPECT_EQ(3U, session()->GetTotalResourceCount());
+ EXPECT_EQ(2U, session()->GetTotalResourceCount());
EXPECT_EQ(1U, session()->GetMappedResourceCount());
// Releasing node 1 causes nodes 1-3 to be destroyed.
diff --git a/garnet/lib/ui/gfx/tests/snapshotter_unittest.cc b/garnet/lib/ui/gfx/tests/snapshotter_unittest.cc
index dd892a8..e49c8ec 100644
--- a/garnet/lib/ui/gfx/tests/snapshotter_unittest.cc
+++ b/garnet/lib/ui/gfx/tests/snapshotter_unittest.cc
@@ -33,7 +33,6 @@
EXPECT_TRUE(Apply(scenic::NewCreateEntityNodeCmd(kParentId)));
EXPECT_TRUE(Apply(scenic::NewSetLabelCmd(kParentId, "Parent")));
EXPECT_TRUE(Apply(scenic::NewCreateShapeNodeCmd(kChildId)));
- EXPECT_TRUE(Apply(scenic::NewAddPartCmd(kParentId, kChildId)));
const ResourceId kMaterialId = nextId++;
EXPECT_TRUE(Apply(scenic::NewCreateMaterialCmd(kMaterialId)));
diff --git a/sdk/lib/ui/scenic/cpp/commands.cc b/sdk/lib/ui/scenic/cpp/commands.cc
index 881129f..a4f5db8 100644
--- a/sdk/lib/ui/scenic/cpp/commands.cc
+++ b/sdk/lib/ui/scenic/cpp/commands.cc
@@ -589,17 +589,6 @@
return command;
}
-fuchsia::ui::gfx::Command NewAddPartCmd(uint32_t node_id, uint32_t part_id) {
- fuchsia::ui::gfx::AddPartCmd add_part;
- add_part.node_id = node_id;
- add_part.part_id = part_id;
-
- fuchsia::ui::gfx::Command command;
- command.set_add_part(std::move(add_part));
-
- return command;
-}
-
fuchsia::ui::gfx::Command NewDetachCmd(uint32_t id) {
fuchsia::ui::gfx::DetachCmd detach;
detach.id = id;
diff --git a/sdk/lib/ui/scenic/cpp/resources.cc b/sdk/lib/ui/scenic/cpp/resources.cc
index 70c5f28..f76e04a 100644
--- a/sdk/lib/ui/scenic/cpp/resources.cc
+++ b/sdk/lib/ui/scenic/cpp/resources.cc
@@ -226,10 +226,6 @@
session()->Enqueue(NewAddChildCmd(id(), child_node_id));
}
-void ContainerNode::AddPart(uint32_t part_node_id) {
- session()->Enqueue(NewAddPartCmd(id(), part_node_id));
-}
-
void ContainerNode::DetachChildren() { session()->Enqueue(NewDetachChildrenCmd(id())); }
EntityNode::EntityNode(Session* session) : ContainerNode(session) {
diff --git a/sdk/lib/ui/scenic/cpp/resources.h b/sdk/lib/ui/scenic/cpp/resources.h
index b951415..629df74 100644
--- a/sdk/lib/ui/scenic/cpp/resources.h
+++ b/sdk/lib/ui/scenic/cpp/resources.h
@@ -257,12 +257,6 @@
}
void AddChild(uint32_t child_node_id);
- void AddPart(const Node& part) {
- ZX_DEBUG_ASSERT(session() == part.session());
- AddPart(part.id());
- }
- void AddPart(uint32_t part_node_id);
-
// Detaches all children from the node.
void DetachChildren();
diff --git a/sdk/lib/ui/scenic/cpp/scenic_cpp.api b/sdk/lib/ui/scenic/cpp/scenic_cpp.api
index a0f3b44..aa8874b 100644
--- a/sdk/lib/ui/scenic/cpp/scenic_cpp.api
+++ b/sdk/lib/ui/scenic/cpp/scenic_cpp.api
@@ -1,7 +1,7 @@
{
"pkg/scenic_cpp/include/lib/ui/scenic/cpp/commands.h": "87b24b4cd022e738edfb8fafa37e6e4f",
"pkg/scenic_cpp/include/lib/ui/scenic/cpp/id.h": "4442d6e024bba018b7a8038d0a069713",
- "pkg/scenic_cpp/include/lib/ui/scenic/cpp/resources.h": "72ea0aa1e9d3b39c7f7d24fd92ac58c9",
+ "pkg/scenic_cpp/include/lib/ui/scenic/cpp/resources.h": "770e22a9cbc64732ee21396e70ece16c",
"pkg/scenic_cpp/include/lib/ui/scenic/cpp/session.h": "9010e6b7c9f743798ca08643c70634b5",
"pkg/scenic_cpp/include/lib/ui/scenic/cpp/view_ref_pair.h": "649d2d31100232dd82c59715be5f9e78",
"pkg/scenic_cpp/include/lib/ui/scenic/cpp/view_token_pair.h": "d3fa89eef3ee68650f9407dd78c67ac3"
diff --git a/src/media/playback/mediaplayer/test/fakes/fake_session.cc b/src/media/playback/mediaplayer/test/fakes/fake_session.cc
index 864c027..74fe05b 100644
--- a/src/media/playback/mediaplayer/test/fakes/fake_session.cc
+++ b/src/media/playback/mediaplayer/test/fakes/fake_session.cc
@@ -87,7 +87,6 @@
HandleAddChild(command.gfx().add_child().node_id, command.gfx().add_child().child_id);
break;
case fuchsia::ui::gfx::Command::Tag::kAddPart:
- HandleAddPart(command.gfx().add_part().node_id, command.gfx().add_part().part_id);
break;
case fuchsia::ui::gfx::Command::Tag::kSetMaterial:
HandleSetMaterial(command.gfx().set_material().node_id,
@@ -248,53 +247,6 @@
child->parent_ = parent_id;
}
-void FakeSession::HandleAddPart(uint32_t node_id, uint32_t part_id) {
- Resource* node = FindResource(node_id);
- Resource* part = FindResource(part_id);
-
- if (!node) {
- FXL_LOG(ERROR) << "Asked to add part, node_id (" << node_id
- << ") not recognized, closing connection.";
- expected_ = false;
- binding_.Unbind();
- return;
- }
-
- if (!node->can_have_children()) {
- FXL_LOG(ERROR) << "Asked to add part, node_id (" << node_id
- << ") can't have children, closing connection.";
- expected_ = false;
- binding_.Unbind();
- return;
- }
-
- if (!part) {
- FXL_LOG(ERROR) << "Asked to add part, part_id (" << part_id
- << ") not recognized, closing connection.";
- expected_ = false;
- binding_.Unbind();
- return;
- }
-
- if (!part->can_be_part()) {
- FXL_LOG(ERROR) << "Asked to add part, part_id (" << part_id
- << ") can't be a part, closing connection.";
- expected_ = false;
- binding_.Unbind();
- return;
- }
-
- if (part->parent_ != kNullResourceId) {
- Resource* prev_parent = FindResource(part->parent_);
- if (prev_parent != nullptr) {
- prev_parent->children_.erase(part_id);
- prev_parent->parts_.erase(part_id);
- }
- }
-
- node->parts_.insert(part_id);
-}
-
void FakeSession::HandleSetMaterial(uint32_t node_id, uint32_t material_id) {
Resource* node = FindResource(node_id);
Resource* material = FindResource(material_id);
diff --git a/src/media/playback/mediaplayer/test/fakes/fake_session.h b/src/media/playback/mediaplayer/test/fakes/fake_session.h
index c740d9c..efe1782 100644
--- a/src/media/playback/mediaplayer/test/fakes/fake_session.h
+++ b/src/media/playback/mediaplayer/test/fakes/fake_session.h
@@ -123,8 +123,6 @@
void HandleAddChild(uint32_t parent_id, uint32_t child_id);
- void HandleAddPart(uint32_t parent_id, uint32_t part_id);
-
void HandleSetMaterial(uint32_t node_id, uint32_t material_id);
void HandleSetTexture(uint32_t material_id, uint32_t texture_id);
