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 general world timer events #1324

Merged
merged 4 commits into from
Nov 1, 2014

Conversation

msteiger
Copy link
Member

Currently, only 4 timer events are fired per day. This PR increases the rate to 100 (adjustable) and uses more the general event class to avoid the need to catch different event types (for the same thing).

The original semantics is still available through method calls.

@MarcinSc
Copy link
Contributor

What is the purpose of the WorldTimeEvent? If a mod wants to be notified about the time updates, it can do it's thing in the "update" method. Not sure if this is really necessary.

@msteiger
Copy link
Member Author

Are you talking about the update method in UpdateSubscriberSystem?

World timer events existed before. My motivation for this implementation was that different modules are interested in similar high-level timer events (e.g. music triggers when the night comes).

Of course, every module could compute the timings itself, but using a (commonly used) timer event is much more convenient.

Related: #1091

@immortius
Copy link
Member

It would be nice if there was a mechanism to subscribe for different time
periods of events, perhaps using the cron format.

Anyhow, I would have preferred the previous 4 events kept as subclasses of
the WorldTimeEvent, so that backwards compatibility was maintained.

On 28 October 2014 20:47, Martin Steiger [email protected] wrote:

Are you talking about the update method in UpdateSubscriberSystem?

World timer events existed before. My motivation for this implementation
was that different modules are interested in similar high-level timer
events (e.g. music triggers when the night comes).

Of course, every module could compute the timings itself, but using a
(commonly used) timer event is much more convenient.

Related: #1091 #1091


Reply to this email directly or view it on GitHub
#1324 (comment)
.

@msteiger
Copy link
Member Author

I added a method matchesDaily(float) to filter events similar to the old TimerEvent class.

The semantic events are back in, also because they hide the dusk/dawn time instants. Thus, future implementation can change them (based on day of year, etc).

@Cervator
Copy link
Member

I have wanted a better scheduler type setup for a long time. While yes you can do your own thing in update() for an updatesubscriber system there ends up being a bunch of boilerplate code slowing down your execution to your desired time rate. If you can just attach to time events fitting your requirements that's cleaner :-)

A full fledged cron type thing would be perfect, but 100/day (how many seconds between events is that?) is a good start, especially keeping the few "labelled" time events

@Cervator
Copy link
Member

On somewhat of a tangent, it would be interesting to also later support differing time systems for different worlds (planets). Again probably something that could be mapped to "real" time as per some world-specific pattern. Even different time of day / celestial events for scenarios like multi-sun worlds.

Wonder how you would translate / map / define that to a cron-like system

@MarcinSc
Copy link
Contributor

As long that it's understood that there is no guarantee of this event being call exactly N times per day. I'm ok with it.

As for scheduling some tasks - you can already do that using the DelayManager.

Also, for having different time day/night cycles in different worlds, we would need support for different worlds first.

@Cervator
Copy link
Member

Started testing this for merge but it causes a couple minor compile issues in Seasons.

The time change makes a few prior public static finals private, that Seasons and SeasonSystem use. One former constant goes to a method instead.

The constant in Season (WORLD_TIME_OFFSET) doesn't actually seem to be in-use anywhere

@msteiger / @MarcinSc how would you prefer to fix? I could probably have merged it with a quick fix without too much trouble but thought I'd ask :-)

@msteiger
Copy link
Member Author

I provided a quick that adds the constants (not dusk/dawn) to the WorldTime interface.

In the next step, I'd like to shift world time to start at midnight which is more "constant" than dawn time. (separate PR). This should also fix the time offset problem in Seasons.

In the long run, I'd also like to factor out dusk/dawn into a separate system and couple it with the sun pos. in Skysphere.

@Cervator
Copy link
Member

Thanks :-)

Agreed on start time at midnight, since we can still just fast forward and spawn the player at dawn?

Would love to see sun position and skysphere stuff get involved, especially as a precursor to multiple celestials. A set of our oldest open issues relate there - #94, #95, #96, #97 :D

@Cervator Cervator merged commit f641460 into MovingBlocks:develop Nov 1, 2014
Cervator added a commit that referenced this pull request Nov 1, 2014
@Cervator
Copy link
Member

Cervator commented Nov 1, 2014

Merged - after the Seasons fix I found another tinier issue yet in PlantPack and fixed that too: Terasology/PlantPack@689623c

I think that's right, pretty minor, but please check it. Unsure if it is a bad practice to use the WorldTime interface directly :-)

@msteiger msteiger deleted the world-timer-events branch December 27, 2014 12:34
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.

4 participants