Addition of background prop to pokemon

[Petap0w]

Experimental but worked well all day for me here.
Will send "background" in pokemon webhook (either null or int value)

[jfberry]

looks good, subject only to data type issue in db

[Fabio1988]

So testing contained C-Day? Thanks a lot, looks good

[Petap0w]

So testing contained C-Day? Thanks a lot, looks good

Yes good for easy Solosis shundo with background :)

[jfberry]

I'm happy once someone has run this on db mode to prove it works (it looks ok, but...)

[Mygod]

I think the rest of the codebase uses LocationCard though. https://github.com/search?q=repo%3AUnownHash%2FGolbat+locationcard&type=code

[Mygod]

We should consider having consistency so change one to the other. AFAIC only ReactMap uses LocationCard internally currently but is not exposed to the frontend yet so you could consider changing all to Background. This would not have any user-facing consequences.

[jfberry]

We should consider having consistency so change one to the other. AFAIC only ReactMap uses LocationCard internally currently but is not exposed to the frontend yet so you could consider changing all to Background. This would not have any user-facing consequences.

I agree +1 for consistency here. Have no opinion on terminology one way or the other, either works for me

[Fabio1988]

As mentioned from @Mygod we should try to be consistent with other location card props, thanks for the hint

[Mygod]

So we shake on background?

[Petap0w]

@Mygod please confirm changes are ok as I don't know what I am doing.

[Fabio1988]

@Petap0w to reuse the util method we just needed to change to int64, in JSON it doesn't even matter but null.IntFrom requires int64... It's safe to use that type and instead of converting the value we just adapt the function, pushed the updated version

One more comment, wihch would need to change I guess :)

[Mygod]

[P0] Preserve null background when no location card — decoder/pokemon.go:1113-1116
With the new util helper we now call null.IntFrom(util.ExtractBackgroundFromDisplay(...)) (e.g. here and in setPokemonDisplay). When the proto has no LocationCard, ExtractBackgroundFromDisplay returns 0, and null.IntFrom(0) marks the column as valid. Previously we only populated the field when LocationCard != nil, so those records stayed NULL. After this change the majority of encounters will be persisted and webhooked with background = 0, which
is incorrect data and breaks the previous semantics consumers rely on to tell the difference between “no background” and “background id 0”. Please keep returning an invalid null.Int when the card is absent (e.g. only set Background if the card exists).

[Mygod]

Given that 0 represents no location card in GM, we should not have both null and 0 as options in the database creating confusion and unnecessarily complicated logic.

[Mygod]

Actually I suppose GM can send both null and 0 as location card: LocationCard can be null and LocationCard.LocationCard can be 0. In that case, we should try to support both even though I don't think 0 is used.

[Mygod]

Running codex review

[Mygod]

One thing that should be tested is whether location card is sent in wild not just encounter. Since we couldn't test it now, we could merge it first and fix it later if this turns out to be broken.

[jfberry]

One thing that should be tested is whether location card is sent in wild not just encounter. Since we couldn't test it now, we could merge it first and fix it later if this turns out to be broken.

We know the display object is only filled in a limited way in wild overworld contexts. Since we encounter anyway it seems pointless to try to populate from wild - there is only downside to this

Some people do not process wild at all because it doesn’t especially help anything only slows down processing

[Mygod]

When I said wild, I was referring to WildPokemonProto which could be picked up from nearby as well.

[jfberry]

When I said wild, I was referring to WildPokemonProto which could be picked up from nearby as well.

All the GMO based Pokémon have a limited pokemondisplay population, so acting on any potential pre-setting of background here without evidence would be a mistake IMO. It’s far more likely this would end up removing any background capture from an encounter

[Mygod]

AFAIC the only thing that is not populated in PokemonDisplayProto is Shiny which is per-account, so I would think the most natural conjecture is that LocationCard is sent in GMO. Given that this is not visible in the game, there is a good chance that this is how it is implemented.

[Mygod]

From codex:

[P0] Add background column to SQLite schema — sql/50_pokemon_background.up.sql:1-2
The decoder now reads and writes pokemon.Background (e.g. via the INSERT/UPDATE statements in decoder/pokemon.go), so every deployment needs a background column on the pokemon table. The MySQL migration adds it, but the SQLite schema in sql/sqlite/create.sql still lacks this column. Any SQLite install that uses that schema (fresh setups or regenerated databases) will start returning SQL errors as soon as the decoder tries to insert or update a Pokémon. Please add the column to the SQLite schema (and any other canonical schema dumps you maintain) to keep SQLite deployments working.

[jfberry]

From codex:

[P0] Add background column to SQLite schema — sql/50_pokemon_background.up.sql:1-2

I'd be concerned if we used sqlite. But we don't.

[Mygod]

Even if your deployment doesn’t touch SQLite today, this repo still ships sql/sqlite/create.sql as the canonical schema for SQLite users. With the new pokemon.background writes landing in decoder/pokemon.go, any fresh SQLite setup—or anyone regenerating their DB from that script—will start failing inserts/updates immediately. Unless we’re officially dropping SQLite support (which would require ripping out quite a bit more), we still need to add the column in sql/sqlite/create.sql to keep that audience running. Let me know if you’d rather take the “remove SQLite support” path instead.

[jfberry]

Even if your deployment doesn’t touch SQLite today, this repo still ships sql/sqlite/create.sql as the canonical schema for SQLite users.

This file can be deleted, it has not been used since very early trials of in memory
It can be deleted in this PR or separately.

[Mygod]

We should probably also have API support filtering by background too?

[jfberry]

Since gmo arrives much more frequently (and this proto doesn’t even get sent by some systems/modes) we deliberately only took missing data (name, desc, and latterly defenders)

Probably the only bit in this PR that has value is being able to ignore the proto if gyms are turned off. Which I’m not sure anyone does

[Fabio1988]

Since gmo arrives much more frequently (and this proto doesn’t even get sent by some systems/modes) we deliberately only took missing data (name, desc, and latterly defenders)

Probably the only bit in this PR that has value is being able to ignore the proto if gyms are turned off. Which I’m not sure anyone does

I assume you wanted to comment on #315 ?