Skip to content

Commit

Permalink
Implement set_current_time and "fix" memory leak (see TODO)
Browse files Browse the repository at this point in the history
  • Loading branch information
@alanjfs
alanjfs committed Jan 6, 2020
1 parent 36d4edb commit 96cff16
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 91 deletions.
69 changes: 44 additions & 25 deletions Source/Sequentity.inl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,13 @@ struct Event {
using Channel = std::vector<Event>;


/**
* @brief Callbacks
*
*/
using OverlappingCallback = std::function<void(const Event&)>;
using TimeChangedCallback = std::function<void()>;


struct State {
bool playing { false };
Expand All @@ -103,9 +109,11 @@ struct Sequentity {
void drawThemeEditor(bool* p_open = nullptr);

void update();
void clear();

void play();
void step(int time);
void step(int delta);
void set_current_time(int time);
void stop();

/** Find intersecting events
Expand Down Expand Up @@ -134,15 +142,15 @@ struct Sequentity {

public:
// Callbacks
void before_stepped(std::function<void(int time)> func) { _before_stepped = func; }
void after_stepped(std::function<void(int time)> func) { _after_stepped = func; }
void before_time_changed(TimeChangedCallback func) { _before_time_changed = func; }
void after_time_changed(TimeChangedCallback func) { _after_time_changed = func; }

private:
entt::registry& _registry;
int _previousTime { 0 };

std::function<void(int time)> _before_stepped = [](int) {};
std::function<void(int time)> _after_stepped = [](int) {};
TimeChangedCallback _before_time_changed = []() {};
TimeChangedCallback _after_time_changed = []() {};

void _sort();
void _on_new_channel();
Expand Down Expand Up @@ -251,46 +259,57 @@ void Sequentity::_sort() {
});
}


void Sequentity::clear() {
Registry.reset<Channel>();
}

void Sequentity::play() {
_registry.ctx<State>().playing ^= true;
}


void Sequentity::step(int time) {
auto& currentTime = _registry.ctx<State>().currentTime;
void Sequentity::set_current_time(int time) {
auto& state = _registry.ctx<State>();
auto& range = _registry.ctx<State>().range;

// Prevent callbacks from being called unnecessarily
if (currentTime + time == _previousTime) return;
if (time == _previousTime) return;

_before_stepped(currentTime);
_before_time_changed();
_previousTime = time;
state.currentTime = time;
_after_time_changed();
}

currentTime += time;

if (currentTime > range.y()) {
currentTime = range.x();
}
void Sequentity::stop() {
auto& state = _registry.ctx<State>();

else if (currentTime < range.x()) {
currentTime = range.y();
}
set_current_time(state.range.x());
state.playing = false;
}

_previousTime = currentTime;

_after_stepped(currentTime);
void Sequentity::update() {
auto& state = _registry.ctx<State>();
if (state.playing) step(1);
}


void Sequentity::stop() {
step(_registry.ctx<State>().range.x() - _registry.ctx<State>().currentTime);
_registry.ctx<State>().playing = false;
}
void Sequentity::step(int delta) {
auto& state = _registry.ctx<State>();
auto time = state.currentTime + delta;

if (time > state.range.y()) {
time = state.range.x();
}

void Sequentity::update() {
if (_registry.ctx<State>().playing) {
step(1);
else if (time < state.range.x()) {
time = state.range.y();
}

set_current_time(time);
}


Expand Down
100 changes: 34 additions & 66 deletions Source/main.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,14 @@ class Application : public Platform::Application {
explicit Application(const Arguments& arguments);

void drawEvent() override;
void drawRigids();
void drawThemeEditor();
void drawScene();
void drawTransport();
void drawTimeline();
void drawCentralWidget();

void setup();
void update();
void clear();
void reset();
void setup(); // Populate registry with entities and components
void reset(); // Reset all entities
void update(); // Apply active events from Sequentity
void clear(); // Clear all events

auto dpiScaling() const -> Vector2;
void viewportEvent(ViewportEvent& event) override;
Expand Down Expand Up @@ -125,7 +123,7 @@ Application::Application(const Arguments& arguments): Platform::Application{argu

setup();

_sequentity.after_stepped([this](int time) { update(); });
_sequentity.after_time_changed([this]() { update(); });
_sequentity.play();
}

Expand Down Expand Up @@ -243,7 +241,26 @@ void Application::reset() {
}

void Application::clear() {
Registry.reset<Sequentity::Channel>();
_sequentity.clear();

// TODO: This is much too explicit and error prone. Is there a better
// way to assert that when a channel goes away, so does the data?
Registry.view<Sequentity::Channel>().each([](auto& channel) {
for (auto& event : channel) {
if (event.type == TranslateEvent) {
delete static_cast<TranslateEventData*>(event.data);
}

else if (event.type == RotateEvent) {
delete static_cast<RotateEventData*>(event.data);
}

else {
Warning() << "Unknown event type" << event.type << "memory has leaked!";
}
}

This comment has been minimized.

Sign in to view

Copy link
@mosra

mosra Jan 6, 2020 •
edited
Loading

Hmm ... maybe store the data in a unique_ptr with a custom deleter? "Pseudo"code:

event.data = std::unique_ptr<void*, void(void*)>{new T{}, [](void* data) {
    delete static_cast<T*>(data);
});

(Magnum's Pointer doesn't support custom deleters right now, so you need to use the standard one)

This comment has been minimized.

Sign in to view

Copy link
@alanjfs

alanjfs Jan 7, 2020

Author Owner

with a custom deleter?

Ooo perhaps this is the missing piece. I'll experiment with that, thank you!

This comment has been minimized.

Sign in to view

Copy link
@alanjfs

alanjfs Jan 10, 2020

Author Owner

Just making a note for myself the next time I try this.

auto data = std::unique_ptr<void*, void(void*)>(new TranslateEventData{}, [](void* data) {
    delete static_cast<TranslateEventData*>(data);
});

data->offset = input.absolute - position;
data->positions.emplace_back(input.absolute);

Works, but..

auto data = static_cast<std::unique_ptr<void*, void(void*)>>(event.data);

Does not, and I'm having trouble understanding the error message.

C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.23.28105\include\xmemory(1432): error C2207: 'std::_Compressed_pair<_Ty1,_Ty2,false>::_Myval1': a member of a class template cannot acquire a function type                
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.23.28105\include\memory(1921): note: see reference to class template instantiation 'std::_Compressed_pair<_Dx,void **,false>' being compiled                               
        with                                                                                                                 
        [                                                                                                                    
            _Dx=void (void *)                                                                                                
        ]                                                                                                                    
..\..\Source\main.cpp(301): note: see reference to class template instantiation 'std::unique_ptr<void *,void (void *)>' being compiled

This comment has been minimized.

Sign in to view

Copy link
@mosra

mosra Jan 10, 2020

i doubt the cast would ever work, that's an equivalent to either a single-param constructor or a conversion operator, and here you need to give it the deleter function pointer

This comment has been minimized.

Sign in to view

Copy link
@alanjfs

alanjfs Jan 10, 2020

Author Owner

Hm, what did you originally have in mind? How can I store a unique pointer to an anonymous struct, access it, and have it deleted along with its owner?

This comment has been minimized.

Sign in to view

Copy link
@mosra

mosra Jan 11, 2020

Oh, sorry. Extending the definition above:

struct Event {
    int time { 0 };
    int length { 0 };
    int crop[2] { 0, 0 };
    struct Color {
        ...
    } color;

    // Assign a different pair of pointer + deleter after
    std::unique_ptr<void*, void(void*)> data { nullptr, [](void*){} };
    ...
};

And then the event.data = ... assignment I showed above should work I think. That of course assumes whatever is managing the Event instances doesn't require them to be copyable (because it's now just movable).
});

reset();
}

Expand Down Expand Up @@ -321,7 +338,7 @@ void Application::drawTransport() {
}

if (ImGui::DragInt("Time", &sqstate.currentTime, 1.0f, sqstate.range.x(), sqstate.range.y())) {
_sequentity.update();
_sequentity.set_current_time(sqstate.currentTime);
}

if (ImGui::DragInt2("Range", sqstate.range.data())) {
Expand All @@ -348,7 +365,7 @@ void Application::drawTransport() {
}


void Application::drawRigids() {
void Application::drawScene() {
auto& sqstate = Registry.ctx<Sequentity::State>();

ImVec2 size = ImGui::GetWindowSize();
Expand Down Expand Up @@ -407,11 +424,9 @@ void Application::drawRigids() {

Button("Scrub", _activeTool.type == ToolType::Scrub);

// auto relativePosition = Vector2i(Vector2(ImGui::GetIO().MouseDelta));
auto relativePosition = Vector2i(Vector2(ImGui::GetMouseDragDelta(0, 0.0f)));
auto absolutePosition = Vector2i(Vector2(ImGui::GetIO().MousePos - ImGui::GetWindowPos()));

bool anyActive { false };
Registry.view<Name, Position, Orientation, Color, Size>().each([&](auto entity,
const auto& name,
const auto& position,
Expand All @@ -423,45 +438,24 @@ void Application::drawRigids() {
auto imangle = static_cast<float>(orientation);
auto imcolor = ImColor(color.fill);

auto state = SmartButton(name.text, impos, imsize, imangle, imcolor);
auto time = sqstate.currentTime + (sqstate.playing ? 1 : 0);
SmartButton(name.text, impos, imsize, imangle, imcolor);

if (state & SmartButtonState_Pressed) {
if (ImGui::IsItemActivated()) {
Registry.assign<Activated>(entity, sqstate.currentTime);
Registry.assign<Input2DRange>(entity, absolutePosition, relativePosition);
anyActive = true;
}

else if (state & SmartButtonState_Dragged) {
else if (ImGui::IsItemActive()) {
Registry.assign<Active>(entity);
Registry.replace<Input2DRange>(entity, absolutePosition, relativePosition);
anyActive = true;
}

else if (state & SmartButtonState_Released) {
else if (ImGui::IsItemDeactivated()) {
Registry.assign<Deactivated>(entity);
anyActive = true;
}
});

// if (!anyActive) {
// auto global = Registry.ctx<entt::entity>();

// if (ImGui::IsMouseClicked(0)) {
// Registry.assign<Activated>(global, _sequentity.currentTime);
// Registry.assign<Input2DRange>(global, absolutePosition, relativePosition);
// }

// else if (ImGui::IsMouseDragging(0)) {
// Registry.assign<Active>(global);
// Registry.replace<Input2DRange>(global, absolutePosition, relativePosition);
// }

// else if (ImGui::IsMouseReleased(0)) {
// Registry.assign<Deactivated>(global);
// }
// }

Registry.view<Position, Sequentity::Channel, Color>().each([&](const auto& position,
const auto& channel,
const auto& color) {
Expand All @@ -476,28 +470,6 @@ void Application::drawRigids() {
}


void Application::_pollMouse() {
auto global = Registry.ctx<entt::entity>();
auto& sqstate = Registry.ctx<Sequentity::State>();
auto absolutePosition = Vector2i(Vector2(ImGui::GetIO().MousePos));
auto relativePosition = Vector2i(Vector2(ImGui::GetIO().MouseDelta));

if (ImGui::IsMouseClicked(0)) {
Registry.assign<Activated>(global, sqstate.currentTime);
Registry.assign<Input2DRange>(global, absolutePosition);
}

else if (ImGui::IsMouseDragging(0, 0.0f)) {
Registry.assign<Active>(global);
Registry.replace<Input2DRange>(global, absolutePosition, relativePosition);
}

else if (ImGui::IsMouseReleased(0)) {
Registry.assign<Deactivated>(global);
}
}


void Application::drawEvent() {
GL::defaultFramebuffer.clear(GL::FramebufferClear::Color);

Expand All @@ -508,13 +480,9 @@ void Application::drawEvent() {

drawCentralWidget();
drawTransport();
drawRigids();

// if (!Registry.view<Activated>().size() || !Registry.view<Active>().size() || !Registry.view<Deactivated>().size()) {
// _pollMouse();
// }
drawScene();

// Handle any input coming from the above drawRigids()
// Handle any input coming from the above drawScene()
_activeTool.system();

_sequentity.update();
Expand Down

0 comments on commit 96cff16

Please sign in to comment.