ygimenez / Pagination-Utils

A collection of methods to make message pagination with JDA easier.
GNU Lesser General Public License v2.1
27 stars 7 forks source link

Preventing use of complete #22

Closed avurro closed 3 years ago

avurro commented 3 years ago

Now i get the following error: image

This is the line of code mentioned: image

avurro commented 3 years ago

I realized that the problem arises when creating the bot in "light" mode: JDABuilder.createLight

In fact, in this way, when the following method is invoked: paginator.getEmoteMap () you get an empty list.

The list is full if I create the bot in "default" mode: JDABuilder.createDefault

I also noticed that the same code works when loading the Emotes related to the arrows. I have no idea what the mistake is, but I think it's a good idea to pre-load all the EMOTEs you are going to use.

I hope I have been helpful

avurro commented 3 years ago

ok.. i found the difference. createLight disables the cache dedicated to EMOTE. Unlike createDefault, the EMOTEs aren't loaded in advance.

With the following code I resolve the error: .enableCache(CacheFlag.EMOTE)

However i would like to avoid loading the EMOTEs, of all the servers in which the bot will enter, into RAM.

So I return to the conclusion that maybe, passing all the EMOTE used in the construction phase (and not just those relating to the arrows) is the best solution.

In order to avoid scrolling through all the guilds in which the bot is present to search for emojis, especially considering that, in the case of thousands of servers, it could become a serious problem.

ygimenez commented 3 years ago

The library has an internal cache for emotes, so it only searches for a given emote once (or if it's no longer valid). This is essential because it's impossible to know emote availability during construction without using JDA's cache, so it's the drawback of going cache-less.

About the complete issue, that's easy to fix, just need to use submit instead, I'll add to the next release.

avurro commented 3 years ago

Wait.. you with this code are looking for EMOTEs in all servers:

for (Guild guild : handler.getGuilds()) {
   try {
      e = guild.retrieveEmoteById(id).complete();
      break;
   } catch (ErrorResponseException ignore) {}
}

Here you are searching all the servers where the bot is present, a very expensive operation if the bot in question is on thousands of servers.

The cache you manage with Pagination is a must. But I am not convinced that this cycle is the best solution.

In the end we talk about a few EMOTEs, why not directly define the IDs in the construction phase of Pagination? (and the guilds where this EMOTEs are loaded if needed)

ygimenez commented 3 years ago

Because the only emotes that are fixed are those from paginate and the "X" button, all the other buttons allow any emote to be used during runtime, making it necessary to declare them during construction would severely limit the possible use-cases for them.

That solution might not be the best solution, but since Discord doesn't point which server an emote belongs to there's really no way to retrieve it just from its ID, it's necessary to find out the source server first.

If you look where getOrRetrieveEmote is called you'll notice that it's only used during method start (for event-handling it's all done through IDs) so it's actually less costly than it appears.

ygimenez commented 3 years ago

Cache has the benefit of drastically reducing API calls, it's one of the reasons it exist. The emote cache from JDA has very little footprint for the amount of benefit it brings, I'd really suggest using it unless you're running on severely limited specs (like heroku).

ygimenez commented 3 years ago

Also, do note that this

Guild g = handler.getGuildById(paginator.getEmoteMap().getOrDefault(id, "0"));

retrieves the guild ID from past retrievals, so it further reduces looping over guilds to find the desired Emote.

ygimenez commented 3 years ago

I've added a way to whitelist server IDs for Emote lookup, this should reduce the amount of places the library will search for the supplied Emote ID.

Both that and the complete() fix are in release 2.1.7