Skip to content
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

[Impeller Scene] Change how property resolution works to fix Animation blending; add mutation log to nodes; enable backface culling; add vertex color contribution back to meshes #38766

Merged
merged 10 commits into from
Jan 12, 2023
27 changes: 21 additions & 6 deletions impeller/scene/animation/animation_player.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,15 @@ AnimationPlayer::~AnimationPlayer() = default;
AnimationPlayer::AnimationPlayer(AnimationPlayer&&) = default;
AnimationPlayer& AnimationPlayer::operator=(AnimationPlayer&&) = default;

AnimationClip& AnimationPlayer::AddAnimation(
std::shared_ptr<Animation> animation,
AnimationClip* AnimationPlayer::AddAnimation(
const std::shared_ptr<Animation>& animation,
Node* bind_target) {
AnimationClip clip(std::move(animation), bind_target);
if (!animation) {
VALIDATION_LOG << "Cannot add null animation.";
return nullptr;
}

AnimationClip clip(animation, bind_target);

// Record all of the unique default transforms that this AnimationClip
// will mutate.
Expand All @@ -31,8 +36,18 @@ AnimationClip& AnimationPlayer::AddAnimation(
{binding.node, binding.node->GetLocalTransform()});
}

clips_.push_back(std::move(clip));
return clips_.back();
clips_.insert({animation->GetName(), std::move(clip)});
Copy link
Member

Choose a reason for hiding this comment

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

std::maps insert or insert_or_assign returns an iterator to element already. So you don't need to insert and then in turn find the inserted element.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

auto found = clips_.find(animation->GetName());
FML_DCHECK(found != clips_.end());
return &found->second;
}

AnimationClip* AnimationPlayer::GetClip(const std::string& name) {
auto result = clips_.find(name);
if (result == clips_.end()) {
return nullptr;
}
return &result->second;
}

void AnimationPlayer::Update() {
Expand All @@ -46,7 +61,7 @@ void AnimationPlayer::Update() {
Reset();

// Update and apply all clips.
for (auto& clip : clips_) {
for (auto& [_, clip] : clips_) {
clip.Advance(delta_time);
clip.ApplyToBindings();
}
Expand Down
8 changes: 5 additions & 3 deletions impeller/scene/animation/animation_player.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@

#pragma once

#include <map>
#include <memory>
#include <optional>
#include <unordered_map>
#include <vector>

#include "flutter/fml/hash_combine.h"
Expand All @@ -29,9 +29,11 @@ class AnimationPlayer final {
AnimationPlayer(AnimationPlayer&&);
AnimationPlayer& operator=(AnimationPlayer&&);

AnimationClip& AddAnimation(std::shared_ptr<Animation> animation,
AnimationClip* AddAnimation(const std::shared_ptr<Animation>& animation,
Node* bind_target);

AnimationClip* GetClip(const std::string& name);
Copy link
Member

Choose a reason for hiding this comment

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

Missing const correctness 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.

Done.


/// @brief Advanced all clips and updates animated properties in the scene.
void Update();

Expand All @@ -41,7 +43,7 @@ class AnimationPlayer final {
private:
std::unordered_map<Node*, Matrix> default_target_transforms_;

std::vector<AnimationClip> clips_;
std::map<std::string, AnimationClip> clips_;

std::optional<TimePoint> previous_time_;

Expand Down
2 changes: 1 addition & 1 deletion impeller/scene/geometry.cc
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ std::shared_ptr<Geometry> Geometry::MakeFromFlatbuffer(
}

DeviceBufferDescriptor buffer_desc;
buffer_desc.size = vertices_bytes * indices_bytes;
buffer_desc.size = vertices_bytes + indices_bytes;
buffer_desc.storage_mode = StorageMode::kHostVisible;

auto buffer = allocator.CreateBuffer(buffer_desc);
Expand Down
57 changes: 55 additions & 2 deletions impeller/scene/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <inttypes.h>
#include <atomic>
#include <memory>
#include <vector>

#include "flutter/fml/logging.h"
#include "impeller/base/strings.h"
Expand All @@ -24,6 +25,24 @@ namespace scene {

static std::atomic_uint64_t kNextNodeID = 0;

void Node::MutationLog::Append(const Entry& entry) {
std::lock_guard<std::mutex> lock(write_mutex_);
dirty_ = true;
entries_.push_back(entry);
}

std::optional<std::vector<Node::MutationLog::Entry>>
Node::MutationLog::Flush() {
if (!dirty_) {
return std::nullopt;
}
std::lock_guard<std::mutex> lock(write_mutex_);
dirty_ = false;
auto result = entries_;
entries_ = {};
return result;
}

std::shared_ptr<Node> Node::MakeFromFlatbuffer(
const fml::Mapping& ipscene_mapping,
Allocator& allocator) {
Expand Down Expand Up @@ -257,7 +276,7 @@ std::shared_ptr<Animation> Node::FindAnimationByName(
return nullptr;
}

AnimationClip& Node::AddAnimation(const std::shared_ptr<Animation>& animation) {
AnimationClip* Node::AddAnimation(const std::shared_ptr<Animation>& animation) {
if (!animation_player_.has_value()) {
animation_player_ = AnimationPlayer();
}
Expand Down Expand Up @@ -287,6 +306,11 @@ Matrix Node::GetGlobalTransform() const {
}

bool Node::AddChild(std::shared_ptr<Node> node) {
if (!node) {
VALIDATION_LOG << "Cannot add null child to node.";
return false;
}

// TODO(bdero): Figure out a better paradigm/rules for nodes with multiple
// parents. We should probably disallow this, make deep
// copying of nodes cheap and easy, add mesh instancing, etc.
Expand Down Expand Up @@ -328,7 +352,32 @@ bool Node::IsJoint() const {

bool Node::Render(SceneEncoder& encoder,
Allocator& allocator,
const Matrix& parent_transform) const {
const Matrix& parent_transform) {
std::optional<std::vector<MutationLog::Entry>> log = mutation_log_.Flush();
if (log.has_value()) {
for (const auto& entry : log.value()) {
if (auto e = std::get_if<MutationLog::SetTransformEntry>(&entry)) {
local_transform_ = e->transform;
} else if (auto e =
std::get_if<MutationLog::SetAnimationStateEntry>(&entry)) {
auto* clip = animation_player_->GetClip(e->animation_name);
if (!clip) {
continue;
}
clip->SetPlaying(e->playing);
clip->SetWeight(e->weight);
clip->SetPlaybackTimeScale(e->time_scale);
} else if (auto e =
std::get_if<MutationLog::SeekAnimationEntry>(&entry)) {
auto* clip = animation_player_->GetClip(e->animation_name);
if (!clip) {
continue;
}
clip->Seek(SecondsF(e->time));
}
}
}

if (animation_player_.has_value()) {
animation_player_->Update();
}
Expand All @@ -345,5 +394,9 @@ bool Node::Render(SceneEncoder& encoder,
return true;
}

void Node::AddMutation(const MutationLog::Entry& entry) {
mutation_log_.Append(entry);
}

} // namespace scene
} // namespace impeller
42 changes: 40 additions & 2 deletions impeller/scene/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#pragma once

#include <memory>
#include <mutex>
#include <optional>
#include <vector>

Expand All @@ -25,6 +26,39 @@ namespace scene {

class Node final {
public:
class MutationLog {
public:
struct SetTransformEntry {
Matrix transform;
};

struct SetAnimationStateEntry {
std::string animation_name;
bool playing;
float weight;
Copy link
Member

Choose a reason for hiding this comment

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

impeller::Scalar here and default initialize the rest?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

float time_scale;
};

struct SeekAnimationEntry {
std::string animation_name;
float time;
};

using Entry = std::
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this just calls for inheritance instead of variants?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm on the fence, but I think that's where it will end up. Will do this in a later patch.

variant<SetTransformEntry, SetAnimationStateEntry, SeekAnimationEntry>;

void Append(const Entry& entry);

private:
std::optional<std::vector<Entry>> Flush();

bool dirty_ = false;
std::vector<Entry> entries_;
std::mutex write_mutex_;
Copy link
Member

Choose a reason for hiding this comment

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

Please use impeller::Mutex, impeller::RWMutex, etc. and the associated static thread safety analysis macros here. That way, the compiler will tell us attempt a data race.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


friend Node;
};

static std::shared_ptr<Node> MakeFromFlatbuffer(
const fml::Mapping& ipscene_mapping,
Allocator& allocator);
Expand All @@ -44,7 +78,7 @@ class Node final {
bool exclude_animation_players = false) const;

std::shared_ptr<Animation> FindAnimationByName(const std::string& name) const;
AnimationClip& AddAnimation(const std::shared_ptr<Animation>& animation);
AnimationClip* AddAnimation(const std::shared_ptr<Animation>& animation);

void SetLocalTransform(Matrix transform);
Matrix GetLocalTransform() const;
Expand All @@ -63,7 +97,9 @@ class Node final {

bool Render(SceneEncoder& encoder,
Allocator& allocator,
const Matrix& parent_transform) const;
const Matrix& parent_transform);

void AddMutation(const MutationLog::Entry& entry);

private:
void UnpackFromFlatbuffer(
Expand All @@ -72,6 +108,8 @@ class Node final {
const std::vector<std::shared_ptr<Texture>>& textures,
Allocator& allocator);

mutable MutationLog mutation_log_;

Matrix local_transform_;

std::string name_;
Expand Down
5 changes: 2 additions & 3 deletions impeller/scene/scene.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ Node& Scene::GetRoot() {
}

bool Scene::Render(const RenderTarget& render_target,
const Matrix& camera_transform) const {
const Matrix& camera_transform) {
// Collect the render commands from the scene.
SceneEncoder encoder;
if (!root_.Render(encoder,
Expand All @@ -57,8 +57,7 @@ bool Scene::Render(const RenderTarget& render_target,
return true;
}

bool Scene::Render(const RenderTarget& render_target,
const Camera& camera) const {
bool Scene::Render(const RenderTarget& render_target, const Camera& camera) {
return Render(render_target,
camera.GetTransform(render_target.GetRenderTargetSize()));
}
Expand Down
4 changes: 2 additions & 2 deletions impeller/scene/scene.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ class Scene {
Node& GetRoot();

bool Render(const RenderTarget& render_target,
const Matrix& camera_transform) const;
const Matrix& camera_transform);

bool Render(const RenderTarget& render_target, const Camera& camera) const;
bool Render(const RenderTarget& render_target, const Camera& camera);

private:
std::shared_ptr<SceneContext> scene_context_;
Expand Down
19 changes: 10 additions & 9 deletions impeller/scene/scene_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,10 @@ TEST_P(SceneTest, TwoTriangles) {
auto animation = gltf_scene->FindAnimationByName("Metronome");
ASSERT_NE(animation, nullptr);

AnimationClip& metronome_clip = gltf_scene->AddAnimation(animation);
metronome_clip.SetLoop(true);
metronome_clip.Play();
AnimationClip* metronome_clip = gltf_scene->AddAnimation(animation);
ASSERT_NE(metronome_clip, nullptr);
metronome_clip->SetLoop(true);
metronome_clip->Play();

auto scene_context = std::make_shared<SceneContext>(GetContext());
auto scene = Scene(scene_context);
Expand All @@ -145,18 +146,18 @@ TEST_P(SceneTest, TwoTriangles) {
ImGui::SliderFloat("Weight", &weight, -2, 2);
ImGui::Checkbox("Loop", &loop);
if (ImGui::Button("Play")) {
metronome_clip.Play();
metronome_clip->Play();
}
if (ImGui::Button("Pause")) {
metronome_clip.Pause();
metronome_clip->Pause();
}
if (ImGui::Button("Stop")) {
metronome_clip.Stop();
metronome_clip->Stop();
}

metronome_clip.SetPlaybackTimeScale(playback_time_scale);
metronome_clip.SetWeight(weight);
metronome_clip.SetLoop(loop);
metronome_clip->SetPlaybackTimeScale(playback_time_scale);
metronome_clip->SetWeight(weight);
metronome_clip->SetLoop(loop);
}

ImGui::End();
Expand Down
Loading