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

0005204: Fix Weather blending at server #204

Merged
merged 1 commit into from Jun 25, 2018

Conversation

Projects
3 participants
@emre1702
Copy link
Contributor

emre1702 commented Jun 24, 2018

Bugtracker:
https://bugs.mtasa.com/view.php?id=5204

The current weather-system at serverside is not so good.
It saves the end-hour of a weather-blending and asks, if the current hour is the end-hour. So to make the serversided blending work, the server has to compare the hours at every hour atleast once.

Currently the comparation of the hours for the weather-blending happens only on "SetSeatherBlended" and "GetWeather". So to not get a bug, we would have to call one of them at the end-hour of the blending (after how many days doesn't matter).

So my "workaround" would be to add a "DoPulse" to check it every interval. This solution fixes the bug without changing much.

@@ -469,6 +469,8 @@ void CGame::DoPulse(void)

CLOCK_CALL1(m_pAsyncTaskScheduler->CollectResults());

CLOCK_CALL1(m_pMapManager->GetWeather()->DoPulse(););

This comment has been minimized.

@qaisjp

qaisjp Jun 25, 2018

Member

DoPulse();); should that extra semicolon be there?

This comment has been minimized.

@emre1702

emre1702 Jun 25, 2018

Author Contributor

It's in nearly every other line with CLOCK_CALL1, so wanted to "continue" this style o.o

This comment has been minimized.

@qaisjp

qaisjp Jun 25, 2018

Member

Oh I see — I was just confused because the one above doesn't use it! Should be fine.

@qaisjp qaisjp added the bug label Jun 25, 2018

@qaisjp

This comment has been minimized.

Copy link
Member

qaisjp commented Jun 25, 2018

It seems like this does a check every tick instead of every hour. Have you explored ways to do this check less frequently? (i.e, not every tick)

@emre1702

This comment has been minimized.

Copy link
Contributor Author

emre1702 commented Jun 25, 2018

Couldn't find a precise way without having to change CClock.cpp a bit.
A "better for performance", but "still propably buggy" other way would be to let CClock call it at the Get method, if the hour changed:

void CClock::Get(unsigned char& ucHour, unsigned char& ucMinute)

A "much cleaner and maybe useful for other things" way would be to check the time every tick and call methods which depend on ingame time.
But that would be worse for performance - but could be better if there are other methods depending on ingame time (are there some?).

@qaisjp

This comment has been minimized.

Copy link
Member

qaisjp commented Jun 25, 2018

You raise some good points. I'm convinced that the "perform pulse on weather get" solution is a good solution (why would you say it's "still probably buggy"?), and would be happy to merge that straight away.

could be better if there are other methods depending on ingame time (are there some?).

Good question. No idea. 🙂

@emre1702

This comment has been minimized.

Copy link
Contributor Author

emre1702 commented Jun 25, 2018

"still probably buggy"
Because the current system is, that we need to call "Update" of CBlendedWeather at the same hour the blending would end. If we don't do that, it needs to be done after 12 hours (etc.), else getWeather will continue to return a wrong result.
If we put the "Update" method in CClocks "Get" method, we still need to make sure, that "Get" gets called atleast once when the hour reached the end-hour.

That's why I wrote:
"The current weather-system at serverside is not so good"
It would be much better if we could check how many hours actually passed, not at which hour we are.
Then we wouldn't need this PR and could call "Update" when calling getWeather etc.
There we could then check if atleast 2 hours passed (instead of checking if the current-hour is the same as the end-hour).
But to do this we would have to change CClock too much, which wouldn't be worth it only for a 100% precise and bugfree "getWeather".

Edit:
"perform pulse on weather get"
No, I meant on clock get.
On weather get is how it currently is, that's why it's buggy.

@qaisjp

This comment has been minimized.

Copy link
Member

qaisjp commented Jun 25, 2018

Ah I think I understand... so the "solution" of putting it in the Get method breaks if it has been a day since the last check (for example).

OK. We can go with what you have currently. At some point, we should do an overall evaluation of what checks need to be done every tick and if we can make certain pulses more infrequent.

@qaisjp qaisjp merged commit 4398a0e into multitheftauto:master Jun 25, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@patrikjuvonen patrikjuvonen added this to In progress in release/v1.5.6 via automation Aug 7, 2018

@patrikjuvonen patrikjuvonen added this to the 1.5.6 milestone Aug 7, 2018

@patrikjuvonen patrikjuvonen moved this from In progress to Done in release/v1.5.6 Aug 7, 2018

@patrikjuvonen patrikjuvonen changed the title #5204 - Fix Weather blending at server 0005204: Fix Weather blending at server Sep 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.