Ticket #46496

Counters: Adjust network protocol

Eröffnet am: 2023-01-09 14:21 Letztes Update: 2023-02-16 03:40

Auswertung:
Verantwortlicher:
Typ:
Status:
Geschlossen
Komponente:
Meilenstein:
Priorität:
5 - Mittel
Schweregrad:
5 - Mittel
Lösung:
Gefixt
Datei:
5

Details

As noted by lachu in #45889, delta protocol currently considers each counter package to replace others - i.e. even unchanged counters get sent because another counter has been sent in between.

Simplest way to fix this would be to make both the "counter owner" (e.g. city id, in case of city counters) and counter id keys, but that would mean num cities * num counters separate packages to be kept. Also, calling send_...() separately for each counter is a bit heavier task than it needs to be. Maybe rearrange network protocol so that entire counter array is sent as a single package (delta protocol would still skip elements that have not changed) and just the owner (city id) is the key - this would *also* mean that city id is sent just once for the entire array, and not once for each separate counter value. Once we have multiple counter_target types, it's probably better to have separate packet type for each than to include the target type in the package (and have that affect how the "owner" id gets interpreted)

Ticket-Verlauf (3/20 Historien)

2023-01-09 14:21 Aktualisiert von: cazfi
  • New Ticket "Counters: Adjust network protocol" created
2023-01-17 20:54 Aktualisiert von: lachu
Kommentar

Reply To cazfi

As noted by lachu in #45889, delta protocol currently considers each counter package to replace others - i.e. even unchanged counters get sent because another counter has been sent in between. Simplest way to fix this would be to make both the "counter owner" (e.g. city id, in case of city counters) and counter id keys, but that would mean num cities * num counters separate packages to be kept. Also, calling send_...() separately for each counter is a bit heavier task than it needs to be. Maybe rearrange network protocol so that entire counter array is sent as a single package (delta protocol would still skip elements that have not changed) and just the owner (city id) is the key - this would *also* mean that city id is sent just once for the entire array, and not once for each separate counter value. Once we have multiple counter_target types, it's probably better to have separate packet type for each than to include the target type in the package (and have that affect how the "owner" id gets interpreted)'

0001-OSDN-TICKET-46496-S-awomir-Lach-slawek-lach.art.pl.patch(6KB)
Done
2023-01-18 02:08 Aktualisiert von: lachu
Kommentar

It requires: !45889

Rebase on top of master?

2023-02-03 03:09 Aktualisiert von: cazfi
Kommentar

+PACKET_CITY_UPDATE_COUNTERS = 514; sc, lsend, is-game-info
+ CITY city; key
+ COUNTER counters[MAX_COUNTERS];

- No need to send MAX_COUNTERS block, as we need and, indeed, fill only game.control.num_counters values.

---

+ /* Needed, because we use delta protocol
+ * We do not need to sending seldom data
+ * with seldom possibility.
+ */
+ memset(packet.counters, 0, sizeof(packet.counters));

- Actually not needed as you overwrite all the values. (once the above change to not have garbage space after game.control.num_counters has been implemented)

2023-02-04 01:49 Aktualisiert von: lachu
Kommentar

Reply To cazfi

+PACKET_CITY_UPDATE_COUNTERS = 514; sc, lsend, is-game-info
+ CITY city; key
+ COUNTER counters[MAX_COUNTERS];
- No need to send MAX_COUNTERS block, as we need and, indeed, fill only game.control.num_counters values.
--- + /* Needed, because we use delta protocol
+ * We do not need to sending seldom data
+ * with seldom possibility.
+ */
+ memset(packet.counters, 0, sizeof(packet.counters));
- Actually not needed as you overwrite all the values. (once the above change to not have garbage space after game.control.num_counters has been implemented)

I do not fully understood. I cannot read how to made array variable-size (depending only on size calculated after ruleset loaded) without introducing new field. As I understood, delta protocol will only sends changed values, so zeroes each value before fill is good - these values will be send only once, but it requires space in fact.

I also had read that no-delta turn off treading 0 as default value, so delta probably will omitting 0 bytes.

Should I introduce num_counters filed to packet and COUNTER counters[MAX_COUNTERS!:num_counters] ? This is only one solution I discovered.

(Edited, 2023-02-04 01:54 Aktualisiert von: lachu)
2023-02-04 02:02 Aktualisiert von: cazfi
Kommentar

Reply To lachu

Should I introduce num_counters filed to packet and COUNTER counters[MAX_COUNTERS!:num_counters] ? This is only one solution I discovered.

Yes, that's the way to do it. Then you would be sending 1 + 1 = 2 integers when there's one counter, and not 20 (MAX_COUNTERS) ones. It might be possible even to omit the num_counters from the package and use game.control.num_counters directly, but then we need to be sure that server and client agree about that value in all cases.

2023-02-04 21:00 Aktualisiert von: lachu
Kommentar

Reply To cazfi

Reply To lachu

Should I introduce num_counters filed to packet and COUNTER counters[MAX_COUNTERS!:num_counters] ? This is only one solution I discovered.

Yes, that's the way to do it. Then you would be sending 1 + 1 = 2 integers when there's one counter, and not 20 (MAX_COUNTERS) ones. It might be possible even to omit the num_counters from the package and use game.control.num_counters directly, but then we need to be sure that server and client agree about that value in all cases.

I asks, because delta protocol cut fields of arrays, which do not change. It sends number of changed field one after one and next these fields, so delta protocol will send this integer. One problem was: we always must send initial values - and (in this case) we send whole array.

2023-02-04 21:28 Aktualisiert von: lachu
Kommentar

Reply To cazfi

Reply To lachu

Should I introduce num_counters filed to packet and COUNTER counters[MAX_COUNTERS!:num_counters] ? This is only one solution I discovered.

Yes, that's the way to do it. Then you would be sending 1 + 1 = 2 integers when there's one counter, and not 20 (MAX_COUNTERS) ones. It might be possible even to omit the num_counters from the package and use game.control.num_counters directly, but then we need to be sure that server and client agree about that value in all cases.

0001-OSDN-TICKET-46496-S-awomir-Lach-slawek-lach.art.pl.patch(2KB)
Solution proposed by cazfi
Done, if this solution is enough. I will see at code how to remove field telling how many entries packet contains, if it were be necessary.
2023-02-05 00:22 Aktualisiert von: cazfi
Kommentar

The patch does not apply to master.

2023-02-05 05:07 Aktualisiert von: lachu
2023-02-05 05:08 Aktualisiert von: lachu
  • File 0001-OSDN-TICKET-46496-S-awomir-Lach-slawek-lach.art.pl.patch (File ID: 11553) is deleted
2023-02-05 05:09 Aktualisiert von: lachu
Kommentar

Reply To cazfi

The patch does not apply to master.

0001-OSDN-TICKET-46496-S-awomir-Lach-slawek-lach.art.pl.patch(6KB)

Now it should apply

My bad. I apply message at top of my patches to test. When I switch to working branch again, I do changes, which was at top. Now, I squash newest changes to oldest (before testing commits) and it apply at top of master. I also update master.

2023-02-12 18:52 Aktualisiert von: cazfi
Kommentar

Some trailing spaces - or rather; supposedly empty lines with spaces.

Attached a screenshot of how you should see that with 'git diff'

2023-02-12 20:20 Aktualisiert von: lachu
Kommentar

Reply To cazfi

Some trailing spaces - or rather; supposedly empty lines with spaces. Attached a screenshot of how you should see that with 'git diff'

2023-02-12 20:20 Updated by: lachu

File 0001-OSDN-TICKET-46496-S-awomir-Lach-slawek-lach.art.pl.patch (File ID: 11608) is attached

Sorry. Style was fixed in this patch.

2023-02-13 01:09 Aktualisiert von: cazfi
  • Verantwortlicher Update from (Keine) to cazfi
  • Lösung Update from Keine to Accepted
2023-02-16 03:40 Aktualisiert von: cazfi
  • Status Update from Offen to Geschlossen
  • Lösung Update from Accepted to Gefixt

Bearbeiten

Please login to add comment to this ticket » Anmelden