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

Timers #196

Merged
merged 1 commit into from Jul 10, 2021
Merged

Timers #196

merged 1 commit into from Jul 10, 2021

Conversation

RicardRC
Copy link
Contributor

No description provided.

Copy link
Member

@AndreaCatania AndreaCatania left a comment

Choose a reason for hiding this comment

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

I'm really happy about this already, there are only some minor things to change.

As you already said in chat we are abusing a bit too much with the find and we should find an alternative solution.

As first thing I would add the function is_active_timer so you can use that function to check if a TimerHandle is valid and active. Then I would implement that function as follow:

bool TimerDatabag::is_active_timer(TimerHandle p_handle) {
    return timers.size() < p_handle ? timers[p_handle] < UINT64_MAX : false;
}

databags/databag_timer.cpp Outdated Show resolved Hide resolved
databags/databag_timer.cpp Outdated Show resolved Hide resolved
databags/databag_timer.cpp Outdated Show resolved Hide resolved
databags/databag_timer.cpp Outdated Show resolved Hide resolved
databags/databag_timer.cpp Outdated Show resolved Hide resolved
databags/databag_timer.cpp Outdated Show resolved Hide resolved
databags/databag_timer.cpp Outdated Show resolved Hide resolved
databags/databag_timer.h Outdated Show resolved Hide resolved
databags/databag_timer.cpp Outdated Show resolved Hide resolved
@AndreaCatania
Copy link
Member

AndreaCatania commented Jun 26, 2021

I forgot to mention that a better place for the TimerDatabag is this directory: https://github.com/GodotECS/godex/tree/main/modules/godot/databags

and you should register the databag here: https://github.com/GodotECS/godex/blob/main/modules/godot/register_types.cpp#L79-L94 to make it available.

@AndreaCatania
Copy link
Member

AndreaCatania commented Jun 26, 2021

The CI failed because the correct way to define the type def should be something like:

namespace godex {
typedef uint64_t Timer; // μs (microseconds) until the timer fires
typedef uint32_t TimerHandle;
};

@RicardRC
Copy link
Contributor Author

I've still got to use the is_active_timer , but its in. I'll use it when we finish defining what to do with the destroyed timers vector, to polish up the logic.

@RicardRC RicardRC force-pushed the timers branch 2 times, most recently from 53f8c2d to fabae9c Compare June 27, 2021 10:35
modules/godot/databags/databag_timer.cpp Show resolved Hide resolved
modules/godot/databags/databag_timer.cpp Outdated Show resolved Hide resolved
modules/godot/databags/databag_timer.h Outdated Show resolved Hide resolved
modules/godot/databags/databag_timer.h Outdated Show resolved Hide resolved
modules/godot/databags/databag_timer.h Outdated Show resolved Hide resolved
modules/godot/databags/databag_timer.h Outdated Show resolved Hide resolved
@RicardRC RicardRC marked this pull request as ready for review June 27, 2021 17:33
@RicardRC RicardRC force-pushed the timers branch 2 times, most recently from 266aec6 to d78aeb9 Compare June 29, 2021 08:40
@RicardRC
Copy link
Contributor Author

Sorry for the force push(es)! Compressed all history, was quite messy, and rebased (makes my end of development easier).

@AndreaCatania
Copy link
Member

Do force push on PR branches is totally fine

@RicardRC RicardRC force-pushed the timers branch 6 times, most recently from 30b397b to 3802364 Compare July 2, 2021 12:53
Copy link
Member

@AndreaCatania AndreaCatania left a comment

Choose a reason for hiding this comment

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

Nice we are getting there :) there are only few small things to address.

However, I think we should also add the System that updates now in the DataBag. We can talk about it in chat if you need help with that, or with the registration process.

modules/godot/databags/databag_timer.cpp Outdated Show resolved Hide resolved
modules/godot/databags/databag_timer.cpp Show resolved Hide resolved
modules/godot/databags/databag_timer.cpp Outdated Show resolved Hide resolved
modules/godot/databags/databag_timer.cpp Outdated Show resolved Hide resolved
@RicardRC RicardRC force-pushed the timers branch 3 times, most recently from db0bf33 to b39b9f4 Compare July 10, 2021 10:00
modules/godot/databags/databag_timer.cpp Outdated Show resolved Hide resolved
tests/test_ecs_databag_timer.h Outdated Show resolved Hide resolved
@RicardRC RicardRC force-pushed the timers branch 3 times, most recently from 8c36350 to edc905e Compare July 10, 2021 12:43
@RicardRC RicardRC force-pushed the timers branch 3 times, most recently from 63b8004 to f4b543a Compare July 10, 2021 13:09
Copy link
Member

@AndreaCatania AndreaCatania left a comment

Choose a reason for hiding this comment

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

Great work!!

@AndreaCatania AndreaCatania merged commit 83879bb into GodotECS:main Jul 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants