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

Adding in Timer #17

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

Adding in Timer #17

wants to merge 4 commits into from

Conversation

jwoertink
Copy link
Contributor

This adds in the SDL_Timer stuff. Used for checking elapsed time in milliseconds, and creating delayed callbacks. I added 2 samples for doing a simple timer, and a more advanced timer from the Lazy Foo tutorial 22 & 23.

@@ -0,0 +1,28 @@
module SDL
module Timer

Copy link
Contributor

@wooster0 wooster0 Mar 10, 2018

Choose a reason for hiding this comment

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

Why not just on this line extend self instead of writing self. at every single method?

Copy link
Owner

Choose a reason for hiding this comment

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

I prefer explicit self. definitions over extend self, which only has value if the module is meant to be used as an includable module and a namespace for global methods at the same time —it may be valid here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto ^ 😂

Copy link
Owner

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

Since we have monotonic clocks and evented loops, what do SDL timers provide specifically?

Furthermore, do callbacks run from a Crystal context? I didn't check the documentation.

@jwoertink
Copy link
Contributor Author

I'm not sure since I don't know what a monotonic clock is, but if SDL provides it, then the API should as well. I don't think the callbacks are in a crystal context since they're defined in C. But I didn't need to use any callbacks in the samples.

@ysbaddaden
Copy link
Owner

ysbaddaden commented Mar 14, 2018

Well, SDL provides a network library, which is nice for C (platform abstraction) but they'll mess up with the Crystal event loop. We shouldn't integrate every SDL library, especially when Crystal already has an alternative.

The following will probably behave better (crystal context, integrated into the event loop), for example:

def interval(seconds : Time::Span)
  spawn do
    sleep(seconds)
    yield
  end
end

interval(5.milliseconds) do
  # ...
end

Instead of SDL ticks, there is ticks = Time.monotonic.total_milliseconds and span = Time.measure { ... } which won't be affected by time fluctuations (e.g. changing the computer's date) for computing elapsed time.

end

def self.delay(ms : UInt32)
LibSDL.delay(ms)
Copy link
Owner

Choose a reason for hiding this comment

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

SDL_Delay() duplicates sleep() and AFAIK block the current thread, thus block the crystal event loop.

module Timer

def self.add(interval, &callback : UInt32 -> Void)
LibSDL.add_timer(interval, callback)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm afraid callback will be run from an unknown context, possibly another thread (unknown to crystal, thus unknown to the garbage collector, and incapable to access the event loop).

It also relies on a captured block passed to a C function, which will make it impossible to rely on closure variables, and there may be GC issues (e.g. callback trying to access collected variables) making it complex to handle, over a simple crystal function, such as spawn { sleep(seconds), yield } for example.

start_prompt_surface = font.render_shaded("Press S to Start or Stop the Timer", font_color, bg_color)
pause_prompt_surface = font.render_shaded("Press P to Pause or Unpause the Timer", font_color, bg_color)

class SampleTimer
Copy link
Owner

Choose a reason for hiding this comment

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

This sample timer is very nice, but can be achieved using Time.monotonic and doesn't need SDL_GetTicks. For example:

ticks = Time.monotonic.total_milliseconds

@jwoertink
Copy link
Contributor Author

Ah, I see! Ok. That makes a lot of sense. If that's the case, then maybe we can just have the sample timer run the same way, but with a crystal context, and then just ditch the whole SDL Timer addition? If you're cool with that, I will make that change. Keeping the samples with the same result, but using straight Crystal, and remove the timer.

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.

None yet

3 participants