search

zaksabeast / CaptureSight

Tesla Overlay to view Pokemon game info in Sword, Shield, Brilliant Diamond, Shining Pearl, and Legends Arceus!
GNU General Public License v3.0
156 stars 28 forks source link

Forced shiny raids not showing up correctly #46

Closed zaksabeast closed 4 years ago

zaksabeast commented 4 years ago

I had a report that raid Pokemon forced to be shiny aren't showing up correctly.

This should be a pretty easy fix:

If anyone is interested in taking this on, please let me know! I'm focused on a different CaptureSight feature for a bit.

Leanny commented 4 years ago

Technically it doesnt need to be renamed, does it? I just renamed it to be clear. Pknx for example still uses the old name and also works with it, so might be worth not to rename it

zaksabeast commented 4 years ago

You're right that it doesn't need to be renamed, but I think it's good to do for the same reason we try to give any variable a descriptive name.

Good names help make the code more readable for the future, and serves as good documentation for anyone who wants to borrow code. For example, it was easier to find the named field in your plugin than to read through lots more code to find which field caused specific behaviors.

In my opinion, easier future maintenance (for old and new contributors), better documentation, and the low development cost definitely makes the change valuable.

Edit: Using the same name isn't necessary, however the flatbuffer documents a specific structure in the game, and consistent game documentation between projects also holds value. For example, two projects with similar documentation makes it easier to search for specific terms and files for cross referencing.

Real96 commented 4 years ago

I was just going to open this issue xD

Real96 commented 4 years ago

You should consider also the case in which the seed gives you a shinable PID but it is forced to be not shiny (maybe you already add this when first Charizard dens where released)

https://github.com/Admiral-Fish/RaidFinder/blob/33ef42f31bab31267f1e15b17b327f089186c13e/Core/Generator/RaidGenerator.cpp#L78

zaksabeast commented 4 years ago

This issue should be fixed with https://github.com/zaksabeast/CaptureSight/commit/f8fda119ad261f215b42c0b4dac2bf3f24cd66a3.

@Real96 since you're the one who originally alerted me to the issue, could you please test and make sure it works for you?

Real96 commented 4 years ago

This issue should be fixed with f8fda11.

@Real96 since you're the one who originally alerted me to the issue, could you please test and make sure it works for you?

Yeah sure! There is a case in which the seed should generate a star shiny, so pid is forced to square. Did you consider also this case? There is a little pid modification, in the last number of his upper half

zaksabeast commented 4 years ago

Thanks for testing :slightly_smiling_face:

There is a case in which the seed should generate a star shiny, so pid is forced to square. Did you consider also this case?

CaptureSight doesn't actually use/display the final PID of a raid Pokemon, so that's not something I'm super concerned with. It should still be able to calculate what type of shiny the Pokemon will be, but won't create the final PID of a raid Pokemon based on the shiny type.

RaidPokemon does implement a way to get the PID for the PKM inheritance, so CaptureSight should either:

I'm leaning towards the second option since the extra logic wouldn't benefit CaptureSight in any way and would be more to maintain.

Real96 commented 4 years ago

Thanks for testing

There is a case in which the seed should generate a star shiny, so pid is forced to square. Did you consider also this case?

CaptureSight doesn't actually use/display the final PID of a raid Pokemon, so that's not something I'm super concerned with. It should still be able to calculate what type of shiny the Pokemon will be, but won't create the final PID of a raid Pokemon based on the shiny type.

RaidPokemon does implement a way to get the PID for the PKM inheritance, so CaptureSight should either:

  • Implement the extra logic for documentation/future use purposes
  • Stub out getPID (since it's not being used) and leave a comment with an explanation of what the game normally does here

I'm leaning towards the second option since the extra logic wouldn't benefit CaptureSight in any way and would be more to maintain.

I completely forgot about the PID not being showed!

zaksabeast commented 4 years ago

I ended up adding it as part of consolidating shared PKM methods in https://github.com/zaksabeast/CaptureSight/commit/3c45d34db21f7e437c2aa71fc12be7181fb86847.

Real96 commented 4 years ago

Going to test it in a 20 min! Sorry for being late >.<

Real96 commented 4 years ago

Ok i tested it. My actual Zeraora seed is a star seed. But in it says that next shiny frame will be 2347 (Star). I think this has to be changed. All seeds will give you a Square shiny. The sprites anyway is seen as shiny

zaksabeast commented 4 years ago

@Real96 Sorry for the late response! I forgot to mention that I pushed a change for this. Does it solve the issue?

zaksabeast commented 4 years ago

I'm closing this since I believe it was reported to be resolved.