mm201 / pkmn-classic-framework

Pokémon application logic for Generation IV and V, including servers
http://pkmnclassic.net/
Other
219 stars 43 forks source link

fully support pokemon wifi-plaza's #123

Closed Mythra closed 6 months ago

Mythra commented 1 year ago

fixes #7

I've tested all the bits individually (and that they match what my rust code does which is fully confirmed working). However, since there's no linux support I can't test this web server with the rest of my stack easily. Everything should all work and I've double checked each class individually on my side, but we should still probably get a double check from mm :)

mm201 commented 9 months ago

I'm trying to merge this but I'm getting lots of breakage in DataMysql because of changes to the TrainerProfilePlaza API. I think you changed the cutoff in the structure where the header ends and the body begins. This might be a very justified change, but it breaks compatibility with the existing database and I want to understand exactly what's going on before I accept it.

I would also prefer to keep my take on PlazaSchedule, mainly because it fits the style better. I think it does the same as yours.

PlazaQuestionnaire looks fine as far as I can tell without testing.

Mythra commented 9 months ago

Hey again 😄 ,

I didn't fully realize the TraininerProfilePlaza was written directly in a breaking way, I can look at writing a migration, or restoring the previous field (even if it makes things awkward). The main thing it makes awkward is there are fields that currently are "half in", "half out" the current header structure. The big thing field here across that barrier is the mac address, currently the code looks like:

    public PhysicalAddress MacAddres
    {
      get
      {
        return new PhysicalAddress(new byte[] {
                    GamestatsHeader[8], GamestatsHeader[9], GamestatsHeader[10],
                    GamestatsHeader[11], GamestatsHeader[12], GamestatsHeader[13],
                });
      }
    }

As opposed with the current implementation it would look something like:

    public PhysicalAddress MacAddres
    {
      get
      {
        return new PhysicalAddress(new byte[] {
                    GamestatsHeader[8], GamestatsHeader[9], GamestatsHeader[10],
                    GamestatsHeader[11], Body[0], Body[1],
                });
      }
    }

This also means if we ever wanted to extract the header into a very specific type (that is reused everywhere), it'd be impossible to do so. It would also continue making the code awkward, and at an offset compared to other implementations. This isn't the end of the world if it makes the migration tough I can rework if we want, but this is why I did it this way.


I would also prefer to keep my take on PlazaSchedule, mainly because it fits the style better. I think it does the same as yours.

Yeah! Happy to change this, I know we both made changes at similar times. Happy to rebase and match your style.

mm201 commented 9 months ago

The main thing it makes awkward is there are fields that currently are "half in", "half out" the current header structure. The big thing field here across that barrier is the mac address, currently the code looks like

Yeah, that's really bad. I most likely pasted the code from one of the other setProfile handlers and it was "close enough" to work. It seems like the header/body distinction doesn't even make sense here. I'll look at getting the database schema updated but this will also mean more merge conflicts...

mm201 commented 6 months ago

Merged, but with a lot of customization. I'm keeping my trainer profile and schedule implementation, with your suggested improvements and documentation, and merging your questionnaire implementation. Thank you!