IITC-CE / ingress-intel-total-conversion

intel.ingress.com total conversion user script with some new features. Should allow easier extension of the intel map.
https://iitc.app
ISC License
285 stars 110 forks source link

genPostData should use a different timestamp from storageHash.oldestTimestamp when getOlderMessages = True. #364

Closed perringaiden closed 3 years ago

perringaiden commented 4 years ago

Currently IITC uses the stored oldest timestamp of received messages as the maxTimestampMs in the data for retrieving messages when getOlderMessages is true.

data = $.extend(data, {maxTimestampMs: storageHash.oldestTimestamp});

However, where a single request results in 50+ (or a large value, unknown specific) messages all at the same time, say when a heavily layered field set is destroyed, this means that the first request sets the oldestTimestamp to that time, but every subsequent request keeps requesting the same time period.

Since the chat never receives older messages, it logjams at this timestamp and won't display any earlier entries.

Suggested Solution:

Use the timestamp of the oldestTimestamp minus one millisecond. Since Niantic uses end inclusive timestamps in their request, you need to shift the end timestamp back by one millisecond or you will continue to receive the same data over and over.

data = $.extend(data, {maxTimestampMs: storageHash.oldestTimestamp - 1});
perringaiden commented 4 years ago

Example Request:

{"minLatE6":33837912,"minLngE6":-117942953,"maxLatE6":33905400,"maxLngE6":-117778158,"minTimestampMs":-1,"maxTimestampMs":1593087064406,"tab":"all","v":"bb473cd62691894df0733d10860034fcbeb81897"}

Example Response: InstantResponse.txt

This request kept being repeated, because the max timestamp never moved earlier.

johnd0e commented 4 years ago

Why not open PR?

MysticJay commented 4 years ago

Why not open PR?

What an effort for a two byte change...

I'll integrate it to UNIQUES040 PR as I need that anyway.

MysticJay commented 4 years ago

Cherry-picked the change to #389 as well

Looka13 commented 3 years ago

Using storageHash.oldestTimestamp - 1 as the maxTimestampMs would NOT be a correct solution, and I'll explain why:

Let's imagine that the server has x > 50 messages relative to the timestamp y. If we ask the server for y as the maxTimestampMs we will receive 50 of them instead of x of them (and that's how it currently works). If instead we ask the server for y-1 as the maxTimestampMs we will receive only messages older then the y timestamp, so all the x messages will be "lost". That would be better wrt the current situation, because it would at least let the user scroll to older posts than y, but it would not be the ideal solution.

In my opinion the solution should be implemented in a similar way as how the stock Intel handles the request, which is by using the plextContinuationGuid field in the request (with storageHash.oldestTimestamp as maxTimestampMs). In this way the server "keeps track" of which messages (with timestamp y) has already sent, and replies with "different" messages each time, allowing the user to retrieve all the x messages with timestamp y instead of only 50 of them.

The plextContinuationGuid field requires the GUID of either the "first" or the "last" message the server sent (I'm sorry, I don't remember which one, I made some tests some months ago and I don't remember the answer). The solution should look something like this:

data = $.extend(data, {maxTimestampMs: storageHash.oldestTimestamp, plextContinuationGuid: storageHash.oldestGUID});

Sorry if my English isn't perfect.

Looka13 commented 3 years ago

[Ofc the solution is not complete, it would need to save in the storageHash the GUID of the oldest message received somewhere else in the code (probably in window.chat.writeDataToHash). It is just a suggestion because I'm not a programmer and thus I'm sure there exist better solutions than whichever I would ever come up with.]

Looka13 commented 3 years ago

Sorry if I keep commenting, but I just realized that the solution proposed by @perringaiden may be worse than I initially thought. In one of my previous messages I said that using storageHash.oldestTimestamp - 1 as the maxTimestampMs would result in skipping all the messages with the same timestamp if they are more than 50. It is not so frequent to find more than 50 messages all with the same timestamp, so that would be a somewhat rare case. But, I just realized that the initially proposed solution affects many more cases. For example, if a specific timestamp (let's call it y, as in one of my previous messages) has only two messages (which is quite common, think for example to the use of a virus, which creates as many messages with the same timestamp as the number of the resonators present on the portal), it may happen that one of the two appears as last in a batch of 50 messages received from the server, and since the successive request from the client would ask y-1 as maxTimestampMs, the server would reply with messages older than y, skipping entirely the second of the two messages with y as timestamp.

In short, using storageHash.oldestTimestamp - 1 as the maxTimestampMs can potentially lead to skip many messages in the COMM.

johnd0e commented 3 years ago

What an effort for a two byte change...

I'll integrate it to UNIQUES040 PR as I need that anyway.

I hope now you see your mistake (never mix up unrelated changes in single commit / PR).

MysticJay commented 3 years ago

@johnd0e Well, it was not done that way....

johnd0e commented 3 years ago

I hope now you see your mistake (never mix up unrelated changes in single commit / PR).

I mean cbb38684b0122d18ae7a9a2c8d136d4bb4844a62 of #389

MysticJay commented 3 years ago

@johnd0e I don't remember who proposed it. The changes are very small. A direct commit to master was not wanted, single PRs cause overhead. Creating one commit per file was a reasonable option. The PR should be merged by cherry-picking each commit (which have been crafted to only effect one file at a time). But we can certainly split them up again in PRs with all the overhead for a 2-byte change.

There is always a dark side to everything!

johnd0e commented 3 years ago

Creating one commit per file was a reasonable option.

Wrong. One set of related changes per commit. Whatever number of files. That is in our wiki.

But we can certainly split them up again in PRs with all the overhead for a 2-byte change.

That's exactly what I'd suggest you to prefer next time.

johnd0e commented 3 years ago

@Looka13

Thank you for this investigation!

Looka13 commented 3 years ago

@johnd0e Thank you and all the other developers who works on such an important tool as it is IITC!