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

wip support [time_area] with no specific timezone #4601

Open
wants to merge 2 commits into
base: master
from

Conversation

@gfgtdf
Copy link
Contributor

gfgtdf commented Nov 25, 2019

now the c++ code does no longer add a dummy time when
there are no times defines for an [time_area]. this is
in particular useful when one wants to define areas in
the map editor that is then used ingame for exampel in
filters.
We should consider renamaing [time_area] to just [area]
[time_area]s that dont define their own times are now
ignored during tod calculation.

gfgtdf added 2 commits Nov 25, 2019
now the c++ code does no longer add a dummy time when
there are no times defines for an [time_area]. this is
in particular useful when one wants to define areas in
the map editor that is then used ingame for exampel in
filters.
We should consider renamaing [time_area] to just [area]
[time_area]s that dont define their own times are now
ignored during tod calculation.
@CelticMinstrel

This comment has been minimized.

Copy link
Member

CelticMinstrel commented Nov 26, 2019

Looks nice! However I think it would be best if possible to rework the logic further so that times_nonempty() isn't needed.

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

gfgtdf commented Nov 30, 2019

Looks nice! However I think it would be best if possible to rework the logic further so that times_nonempty() isn't needed.

I could do that, but for that i'd need to figure out in particular how calculate_best_liminal_bonus is supposed to work. In fact i think the liminal calculation is quite broken as it calculates some limial_bonus_ value that neither considers [time_area]s nor is it updated when the time scedule changes and i didnt really want to turn this into a 'fix liminal' pr.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

CelticMinstrel commented Nov 30, 2019

I could do that, but for that i'd need to figure out in particular how calculate_best_liminal_bonus is supposed to work.

If the issue is simple that the calculation requires at least one time, you could just make it return 0 without calculating when the schedule is empty. (Or would that break something?)

In fact i think the liminal calculation is quite broken as it calculates some limial_bonus_ value that neither considers [time_area]s nor is it updated when the time scedule changes

This sounds like it could be something to look into, maybe we should open a new issue for it?

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

GregoryLundberg commented Nov 30, 2019

Defining a named area sounds like a good idea, but should be a new PR.

The area could then be used as, for example, a filter condition. This would especially work well if the area can be a union of several smaller areas. Consider a weapon special which only works within the named area. I'm thinking things like within 2 tiles of the road between two villages, which could be hard to define and cluster the code.

Even if the same effect can be achieved now, named areas could improve readability and maintainability.

If this PR is to fix a specific bug (sounds like it is), it should go in to snub the error, and "Fix time-of-day bonus/penalty calculations" sounds like it should be a new Issue.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

CelticMinstrel commented Nov 30, 2019

Um, @GregoryLundberg I think you're missing something.

Time areas are already named areas. This PR just allows you to create one that doesn't have its own, separate schedule.

Defining an area as a union of other named areas should already be possible using area=list,of,other,area,ids in the area's filter.

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