search

djhackersdev / minime

ALLNET services for newer Sega arcade games
20 stars 3 forks source link

Add Chunithm events for all versions - [opened] #37

Open icex2 opened 3 years ago

icex2 commented 3 years ago

In GitLab by @Felix on Feb 10, 2021, 03:46

Merges chunithm-events -> master

This MR adds event definitions for all Chunithm versions between 1.00 and 1.41. Not sure if this is a good way to approach this, especially the giant switch statement or the special {VERSION} marker that is replaced via regex.

icex2 commented 3 years ago

In GitLab by @Felix on Feb 14, 2021, 01:36

added 3 commits

Compare with previous version

icex2 commented 3 years ago

In GitLab by @Felix on Feb 14, 2021, 01:37

I did a review of my own code again and noticed I forgot to fix the call to startupUri in src/allnet.ts to pass the version info along.

icex2 commented 3 years ago

In GitLab by @Felix on Feb 14, 2021, 01:37

Not sure if I should make version a required parameter.

icex2 commented 3 years ago

In GitLab by @Mai on Feb 14, 2021, 06:21

Based on comments within !30 this does seem to remove legacy event support for those that play with 'omnimix' data. I would like to propose this be considered giving neets who no-life this game more content to grind on

icex2 commented 3 years ago

In GitLab by @Felix on Feb 14, 2021, 06:22

I suggest that those omnimix builds have the always open flag set on each event.

<alwaysOpen>true</alwaysOpen>
icex2 commented 3 years ago

In GitLab by @Felix on Feb 14, 2021, 06:24

resolved all threads

icex2 commented 3 years ago

In GitLab by @Mai on Feb 14, 2021, 06:25

I was assuming this might have been in the game xml data, but your edit did confirm that

icex2 commented 3 years ago

In GitLab by @esterTion on Feb 14, 2021, 08:50

I suggest that those omnimix builds have the always open flag set on each event.

I suggest remove getGameEvent list at all and ask every player just alwaysOpen their needed events, so that everyone is more happy

icex2 commented 3 years ago

In GitLab by @esterTion on Feb 14, 2021, 08:54

Just saying straight now, I don't like your changes made in this merge. It achieves nothing while breaks some players' need.

icex2 commented 3 years ago

In GitLab by @GRIM.657 on Feb 14, 2021, 09:13

If it is removed, everyone is forced to edit their data, which seems not ideal. There are actually users who want to see each version of the game properly, and I think it makes sense for the server to facilitate this. If a user makes a custom omnimix using the option data system, they are already editing their data, and should go ahead and use alwaysOpen where necessary. But for a user who downloads clean data for a particular SDBT version, in my opinion the server should be able to properly respond with the expected events.

icex2 commented 3 years ago

In GitLab by @esterTion on Feb 14, 2021, 09:17

But for a user who downloads clean data for a particular SDBT version, in my opinion the server should be able to properly respond with the expected events.

Still, for a clean data there's no difference between sending all events versus versioned events. It achieves nothing but breaks some players previous working setup

icex2 commented 3 years ago

In GitLab by @GRIM.657 on Feb 14, 2021, 09:24

I think there was a situation encountered recently where sending all IDs caused the game to break, but I don't have the details.

icex2 commented 3 years ago

In GitLab by @esterTion on Feb 14, 2021, 09:29

Oh, that must've been network buffer overflow. CHUNITHM have only 100kb buffer for decompressed data.
This previously affected player scores being lost after 315 charts played.
Mine event list with removed information event still have plenty of space left.

icex2 commented 3 years ago

In GitLab by @GRIM.657 on Feb 14, 2021, 09:53

Oh yeah, that might have been the issue. I can't say for sure since I wasn't the one experiencing it but if that's true then removing the information events seems viable. But of course, one day there will be too many events and some sort of solution like this MR will be needed, and this seems like a reasonable way of doing it.

icex2 commented 3 years ago

In GitLab by @BemaniWitch on Feb 14, 2021, 15:36

Why not add a new .env variable? CHUNITHM_ALLEVENTS if true, it will load a master omnimix list, could remove the information events to avoid having the buffer overflow. That way everyone is happy and the default will still be close to the real thing.

icex2 commented 3 years ago

In GitLab by @GRIM.657 on Feb 14, 2021, 21:40

I do like that but we will still run into the same problem one day, in the future when we're supporting 1.45 or 1.50 there will be too many events to send them all in one request even with the information events removed.

The buffer overflow is definitely the issue now that I look in more detail. The omni list of events for 1.30 (what minime is returning as of this writing) is already 95kb.

icex2 commented 3 years ago

In GitLab by @BemaniWitch on Feb 14, 2021, 22:15

Have you tried esterTion's de-junked list? I wonder how big that would be inside of the request, but yeah, two more mixes (or less) and we would probably hit the limit again. Tricky situation, I wonder if Chunithm would allow for split up responses? Haven't looked at its netcode at all so far though.

icex2 commented 3 years ago

In GitLab by @Felix on Feb 15, 2021, 01:19

Chunithm does not allow for a split-up events list unlike GetUserItemApi and other paginated APIs. The paginated ones have maxCount and nextIndex in the request.

icex2 commented 3 years ago

In GitLab by @Felix on Feb 15, 2021, 01:19

There's also the case of running unmodified data and I prefer to not modify existing data except when adding new content (like an omnimix for a new version of a game). That is why I made all these events lists version-specific since I ran into the response size limit on 1.45.

icex2 commented 3 years ago

In GitLab by @esterTion on Feb 15, 2021, 02:26

Full event with information removed for 1.50 is 83kb, also chunithm new is on the horizon, they might redo the whole game, or they won't, I don't know.

Having a .env toggle sounds pretty good

icex2 commented 3 years ago

In GitLab by @Felix on Feb 15, 2021, 02:35

By "information removed", you mean the events that only have the information object under substances filled in?

icex2 commented 3 years ago

In GitLab by @esterTion on Feb 15, 2021, 03:20

Yes, basically <type>1</type>.
Every event comes with a information event and a corresponding map/music/quest/duel unlock event

Information just display an "ad" when logging in, while unlock event do the actually job

icex2 commented 3 years ago

In GitLab by @Felix on Feb 15, 2021, 03:40

Okay, so there are 554 event entries across all versions of type 1 and 821 for all other types. That's almost half so a big difference. I doubt they will redo the entire game given that RingEdge maimai and Ongeki use very similar network protocols.

icex2 commented 3 years ago

In GitLab by @esterTion on Feb 15, 2021, 03:47

I doubt they will redo the entire game given that RingEdge maimai and Ongeki use very similar network protocols.

It is, but Ongeki uses Microsoft's C# System library for network part, not their own written client. That's why Ongeki doesn't have such weird problem.
But yeah, I guess if they keep using C++ this will be a problem in the future

icex2 commented 3 years ago

In GitLab by @Felix on Feb 15, 2021, 03:51

I was mostly commenting on the actual network part. For !10, I copied components from the Chunithm implementation and modified them to suit Ongeki. I have not looked at older maimai to see the whole scope so my knowledge is limited to what Aqua has implemented.

Maybe some people might want information pop ups to show up or implement event rotation. Minime is not only used for home systems.

icex2 commented 3 years ago

In GitLab by @esterTion on Feb 15, 2021, 04:00

That being said, current implementation in minime actually prevent these players from seeing anything. They only see information once and that's it. Most likely the very first time they didn't even have option set up correct.
After that upgrading amz to amz+ to crystal, any already created profile can't see any information. So at current stage these information would most likely just annoy every new profile creation with minutes of "ads"

But aqua allow per event start_date can actually make that happen, so I would say this is just the design of minime, it just make things work. Maybe not best way, but just work out of box

icex2 commented 3 years ago

In GitLab by @Felix on Feb 15, 2021, 04:02

And I am working on that, but I wanted to get the groundwork in place and get some feedback.

icex2 commented 3 years ago

In GitLab by @tau on Feb 23, 2021, 17:48

I'd like to move this data into the sqlite db somehow and I discussed a few ways to accomplish this with Felix and others. Expect an MR from me to lay this groundwork in the next few days.