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

Update current weather in gym properties #1225

Open
wants to merge 2 commits into
base: legacy
Choose a base branch
from

Conversation

Pontiky
Copy link

@Pontiky Pontiky commented Feb 12, 2022

The aim of this PR is to get the current weather of the S2 cell where the gym is located to update its weather property.

This change allows webhook tools such PokeAlarm to finally receive the weather ID of the gym when a raid is starting to determine if the pokemon is boosted or not.

@jfberry
Copy link

jfberry commented Apr 21, 2022

Poracle remembers the weather rather than needing an extra query. Is this an option?

@Pontiky
Copy link
Author

Pontiky commented Apr 27, 2022

Poracle remembers the weather rather than needing an extra query. Is this an option?

PokeAlarm could also use cached informations like Poracle soon, mostly to determine the weather. Therefore, this change wouldn't be necessary for PA and Poracle side but as it fixes the weather not updating in gym properties and not sending to webhooks, it's still good to implement.

Copy link
Collaborator

@fosJoddie fosJoddie left a comment

Choose a reason for hiding this comment

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

I see 2 "better" ways of dealing with this

  1. In DbWebhookReader, do not read out weather from weather_boosted_condition but do the s2cell->weather table lookup directly there
  2. Add a cross table for gym->weather cell and use that to join in the right weather directly in sql like so:

new table: gym_weather_cell
columns: gym_id, weather_cell_id

gym.gym_id -> gym_weather_cell.gym_id
gym_weather_cell.weather_cell_id

gym_weather_cell can then be populated with the connection to the right weaather, instead of gyms "sharing" data from weather table arbitary.

Solution # 1 is the fastest, but # 2 will be the best scaling wise

@@ -778,6 +778,15 @@ def gyms(self, origin: str, map_proto: dict):
is_ex_raid_eligible = gym["gym_details"]["is_ex_raid_eligible"]
is_ar_scan_eligible = gym["is_ar_scan_eligible"]
is_in_battle = gym['gym_details']['is_in_battle']
s2_cell_id = S2Helper.lat_lng_to_cell_id(latitude, longitude)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've not checked exactly how much of an impact this does on performance, but it strikes me as a very roundabout way to add weather info to gyms, when the current weather information for any gym is always available through doing the same query in front ends or webhook selects.

The issues I have with this solution is

  • Weather updates will not get registered into the gym column when weather changes
  • Calculating s2 cell id for the weather each gym update adds (a very tiny) overhead + the select from weather is 100% not needed, when any front end could do this every time gym info is needed anyhow

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.

3 participants