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

Add a method to get the accurate time in seconds and a demo project #57

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MattijsKneppers
Copy link

@MattijsKneppers MattijsKneppers commented Jun 15, 2020

Adds a method to get the accurate time in seconds. This provides an alternative to the hostTime member that sometimes seems to result in less fine-grained or more jittery movement.

Also adds a demo plugin that uses this method to display (and test) smooth movement.

Comment on lines 179 to 192
// returns accurate time in seconds since launch of plugin
float secondsSinceLaunch() {
if (m_startTime == std::chrono::steady_clock::time_point::min()) {
m_startTime = std::chrono::high_resolution_clock::now();
return 0;
} else {
auto elapsed = std::chrono::high_resolution_clock::now() - m_startTime;
return (float)(std::chrono::duration_cast<std::chrono::milliseconds>(elapsed).count() / 1000.0);
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find that requesting the start time the first time that the time is requested yields incorrect results on the first call. If the variable were to be a static variable it can be initialized to now(). This way it'll get a timestamp from when static initialization happens, which is much closer to actual launch time than lazily querying it.

Copy link
Author

@MattijsKneppers MattijsKneppers Oct 12, 2020

Choose a reason for hiding this comment

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

Right, as far as I am concerned, this time is only to be used in a relative way, for plugins to calculate movements like in the example plugin. secondsSinceLaunch may not actually be the best name, I'll think of a better name.

On the other hand, I realized that ideally, if hostTime would be more fine-grained, this would be even better for plugins to use, as it would allow offline/non-realtime rendering by a host app. The problem with this though is that using this with the example plugin, the movement in Resolume was very stuttery. Is this a known issue, or is it intended?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In resolume the time is set to the timestamp taken at the start of the current frame in milliseconds. Perhaps you were expecting it to be in seconds? Or if you need additional smoothness you may want to use the framerate setting in the composition settings.

Copy link
Author

@MattijsKneppers MattijsKneppers Oct 17, 2020

Choose a reason for hiding this comment

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

I tried to reproduce my previous finding that hostTime was not fine-grained, but I couldn't. I remember getting steps of 250ms, resulting in very stuttery movement. Perhaps I indeed got a frame rate setting wrong back then, or an issue got fixed in more recent versions of Resolume.

However, I still notice a difference in movement smoothness. Using secondsSinceLaunch() is completely smooth and using hostTime results in slightly more jittery movement. Here are two movies of how this looks on my system (16" MBP, AMD Radeon Pro 5500M) with the latest Arena (7.2.1 rev70674). To try this yourself, just switch to hostTime / 1000.0f at line 51 in SmoothnessTester.cpp and recompile.

Perhaps this is due to the internal method Arena uses to set time though, perhaps it is a little less precise than this new implementation?

Anyway, let me know if you'd like me to further pursue this PR. I can also rework it so that the SmoothnessTester project is included but not secondsSinceLaunch(). It would then serve as a visual benchmark for the host implementation of time, which I imagine can be useful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

on macos resolume uses mach_absolute_time so it's possible that this function returns less accurate values. I would still suggest plugins should use hostTime though as this will keep the frames in sync with that the host thinks the time is, and thus allow for correct animation speed when fixed timing is used (eg clip rendering). If it's less smooth as you mention though (i dont see it in the videos), then i think your example is doing it's work with showing that there's room for improvement in resolume.
I cannot currently merge this pull request due to it having conflicts. Are you perhaps manually copying new files into your own repo rather than merging from this repo's updates?

Copy link
Author

Choose a reason for hiding this comment

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

plugins should use hostTime though when fixed timing is used (eg clip rendering).

That is a good point. I add an extra parameter to the plugin that lets you compare the hostTime approach (default) with this new method, see this movie for how it looks on my system. Would you think it makes sense to include the plugin this way?

I cannot currently merge this pull request due to it having conflicts.

Some edits happened on your master after my last work, so I rebased to your master and it should be possible to merge again now. Note though that I have not tested the Windows implementation yet, I'll do that and let you know if everything is good on my end.

Copy link
Author

Choose a reason for hiding this comment

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

@MennoVink Ok, I updated and tested the Windows project as well, it should work now :)

@MennoVink
Copy link
Collaborator

Needs windows project and have ffgl formatting applied to it

@MattijsKneppers
Copy link
Author

Needs windows project

@MennoVink Would you say that the Windows project included in this PR (see second commit) is not sufficient? If so let me know if there is anything I can change.

and have ffgl formatting applied to it

Happy to do this. What is the formatting standard for ffgl? clang-format perhaps?

@MennoVink
Copy link
Collaborator

I'll try the windows project after the merge conflicts have been solved but i wouldn't know how it could be insufficient?
there's a clang-format file in the repo. besides that, for capitalization what i tend to do is look at previously existing code and just mimic that.

@MattijsKneppers MattijsKneppers force-pushed the showsync/add-accurate-time branch from 7f8a937 to 99a5681 Compare November 7, 2020 14:17
Also add a plugin that uses this method to display (and test) smooth movement.
@MattijsKneppers MattijsKneppers force-pushed the showsync/add-accurate-time branch from 78ca31b to cc275ea Compare November 7, 2020 19:45
@MattijsKneppers
Copy link
Author

there's a clang-format file in the repo.

Thanks, done!

@MattijsKneppers MattijsKneppers force-pushed the showsync/add-accurate-time branch from cebad5a to 523869f Compare November 9, 2020 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants