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 per-player time offset #7002
Conversation
interesting 🤔 |
doc/lua_api.txt
Outdated
@@ -3783,6 +3783,9 @@ This is basically a reference to a C++ `ServerActiveObject` | |||
* `0`...`1`: Overrides day-night ratio, controlling sunlight to a specific amount | |||
* `nil`: Disables override, defaulting to sunlight based on day-night cycle | |||
* `get_day_night_ratio()`: returns the ratio or nil if it isn't overridden | |||
* `set_time_offset(offset)` | |||
* Adds offset to the time of the player (e.g. TimeOfDay is 12:00 and the offset of the player is 1000 then the time of the player is 13:00) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put the example on a separate line
doc/lua_api.txt
Outdated
@@ -3783,6 +3783,9 @@ This is basically a reference to a C++ `ServerActiveObject` | |||
* `0`...`1`: Overrides day-night ratio, controlling sunlight to a specific amount | |||
* `nil`: Disables override, defaulting to sunlight based on day-night cycle | |||
* `get_day_night_ratio()`: returns the ratio or nil if it isn't overridden | |||
* `set_time_offset(offset)` | |||
* Adds offset to the time of the player (e.g. TimeOfDay is 12:00 and the offset of the player is 1000 then the time of the player is 13:00) | |||
* `get_time_offset()` returns the offset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* `get_time_offset()`: returns the players time of day offset
would be better
src/remoteplayer.h
Outdated
m_time_offset = offset; | ||
} | ||
|
||
void getTimeOffset(u16 *offset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason why this isn't u16 getTimeOffset() const
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just return the number instead of using a pointer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used getDayNightRatio as an example because I'm not so good in c++.
src/script/lua_api/l_object.cpp
Outdated
|
||
int offset_i = luaL_checknumber(L, 2); | ||
u16 offset = offset_i; | ||
//Negative values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the offset not just a signed number (s16
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because timeofday is an unsigned number and when I add a negative offset to timeofday and timeofday<0 then it would be the wrong time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either way, an offset of, say, 23999 would effectively work the same, and if anything you could perform a modulus (t % 24000)
src/script/lua_api/l_object.cpp
Outdated
} | ||
|
||
player->setTimeOffset(offset); | ||
getServer(L)->setTimeOfDay(getEnv(L)->getTimeOfDay()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better just resend the time to that one player
src/server.cpp
Outdated
} | ||
else { | ||
NetworkPacket pkt(TOCLIENT_TIME_OF_DAY, 0, peer_id); | ||
pkt << time << time_speed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code does not account for time offsets
player->getTimeOffset(&time_offset); | ||
u16 ntime = (time+time_offset); | ||
pkt << ntime << time_speed; | ||
Send(&pkt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could move the actual implementation into the else
and have this code only call SendTimeOfDay(peer_id, time, time_speed)
for each player
While you're doing this, could you add per-player time speed as well please? |
for (auto &client_name : m_clients.getPlayerNames()) { | ||
RemotePlayer *player = m_env->getPlayer(client_name.c_str()); | ||
peer_id = player->getPeerId(); | ||
NetworkPacket pkt(TOCLIENT_TIME_OF_DAY, 0, peer_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a blank line above here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments
doc/lua_api.txt
Outdated
@@ -3783,6 +3783,10 @@ This is basically a reference to a C++ `ServerActiveObject` | |||
* `0`...`1`: Overrides day-night ratio, controlling sunlight to a specific amount | |||
* `nil`: Disables override, defaulting to sunlight based on day-night cycle | |||
* `get_day_night_ratio()`: returns the ratio or nil if it isn't overridden | |||
* `set_time_offset(offset)` | |||
* Adds offset to the time of the player | |||
* e.g. TimeOfDay is 12:00 and the offset of the player is 1000 then the time of the player is 13:00 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong indents
src/script/lua_api/l_object.cpp
Outdated
//Negative values | ||
if (offset_i<0){ | ||
offset = 24000+offset_i; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove brackets or add a space after )
-> 0) {
src/script/lua_api/l_object.cpp
Outdated
u16 offset = offset_i; | ||
//Negative values | ||
if (offset_i<0){ | ||
offset = 24000+offset_i; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spaces around operator
http://dev.minetest.net/Code_style_guidelines
src/script/lua_api/l_object.cpp
Outdated
} | ||
|
||
player->setTimeOffset(offset); | ||
getServer(L)->SendTimeOfDay(player->getPeerId(), getEnv(L)->getTimeOfDay(), g_settings->getFloat("time_speed")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line >80 characters
player->setTimeOffset(offset); | ||
getServer(L)->SendTimeOfDay(player->getPeerId(), getEnv(L)->getTimeOfDay(), g_settings->getFloat("time_speed")); | ||
|
||
lua_pushboolean(L, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useless return value as the player object must logically be valid and there's no way to fail otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It returns nil if (player == NULL).
src/server.cpp
Outdated
// When the player is joining, player is NULL | ||
if (!player){ | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. Either remove brackets or put a space after )
src/server.cpp
Outdated
|
||
if (peer_id == PEER_ID_INEXISTENT) { | ||
m_clients.sendToAll(&pkt); | ||
|
||
for (auto &client_name : m_clients.getPlayerNames()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::vector<session_t> clients = m_clients.getClientIDs();
for (const session_t peer_id : clients) {
const RemotePlayer *player = m_env->getPlayer(peer_id);
...
}
is faster.
EDIT: player
can be NULL
/nullptr
, so use continue if that applies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works.
@Arcelmi Could you please satisfy LINT's needs so this can be merged after the 2nd approval? https://travis-ci.org/minetest/minetest/jobs/352884874 Testing mod: minetest.register_chatcommand("timediff", {
func = function(name, param)
local player = minetest.get_player_by_name(name)
player:set_time_offset(tonumber(param) or 0);
end
}) |
LINT's requests are unacceptable here. |
@paramat So in that case.. merge this PR without regard of LINT as soon it has two approvals? That would be ok for me too. |
Ok, but best it is added to the whitelist by the author. |
just a note: this totally break mechanics based on time, with that player will have an offset for time based events |
src/script/lua_api/l_object.cpp
Outdated
//Negative values | ||
if (offset_i < 0) { | ||
offset = 24000 + offset_i; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
brace removal, also the comment should explain why it's unsigned
} | ||
|
||
player->setTimeOffset(offset); | ||
getServer(L)->SendTimeOfDay(player->getPeerId(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this should not be done directly from here
instead of making SendTimeOfDay
public, add a method in Serve such as updatePlayerTime()
that does this
src/server.cpp
Outdated
std::vector<session_t> clients = m_clients.getClientIDs(); | ||
for (const session_t peer_id : clients) { | ||
RemotePlayer *player = m_env->getPlayer(peer_id); | ||
if (player) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!player)
continue;
[...]
0e3b135
to
39c54e1
Compare
With this pull request it's possible to change the timeOfDay of a certain player for e.g. 1 hour. So it's possible to make in-game time zones or something like this.
Unlike override_day_night_ratio it also change the position of the moon and sun and not only the sunlight. And it isn't static like override_day_night_ratio and is relative to the timeOfDay.
It's also possible to make machines that change the time in a certain radius and many other possibilities. It's also useful on minigame-servers when players in different minigames have got different daytime.