Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix item menu flicker fix documentation #1680

Closed
wants to merge 10 commits into from
Closed

Fix item menu flicker fix documentation #1680

wants to merge 10 commits into from

Conversation

rawr51919
Copy link
Contributor

@rawr51919 rawr51919 commented Jun 10, 2022

Fixes the documentation for the item menu flicker fix

Description

Implemented the Discord chat suggestions as per #1215 to the item menu flicker fix. Suggest changes to this fix as you wish.
Fixes #1215
Fixes #1514

Discord contact info

chloerawr(female symbol)#5011

Fixes the documentation for the item menu flicker fix
@rawr51919 rawr51919 changed the title Fix both #1215 and #1514 Fix item menu flicker fix documentation Jun 10, 2022
@LOuroboros
Copy link
Contributor

These instructions need a correction.

If you just uncomment the empty struct Sprite at gflib/sprite.h and fill it with elements, the compiiler will error out as it will contradict the other, actually used struct Sprite located a few lines below.

Fix a compile-breaking problem with the flicker fix
@rawr51919
Copy link
Contributor Author

@LOuroboros This should fix that small problem

@LOuroboros
Copy link
Contributor

@LOuroboros This should fix that small problem

agbcc: warnings being treated as errors
In file included from include/pokemon.h:4,
from include/global.h:541,
from src/AgbRfu_LinkManager.c:1:
gflib/sprite.h:174: warning: `struct Sprite' declared inside parameter list
gflib/sprite.h:174: warning: its scope is only this definition or declaration,
gflib/sprite.h:174: warning: which is probably not what you want.>

Wonder if formatting was causing that warning specified by the previous comment
@LOuroboros
Copy link
Contributor

Wonder if formatting was causing that warning specified by the previous comment

I already had the syntax set like that, so it wasn't. When that sort of errors appear, it's 99% of the time a problem with the code. Adding or removing empty spaces won't make things suddenly work.

@rawr51919
Copy link
Contributor Author

Did you ensure you used a fresh copy of this repo with the changes described here in place? Could be you forgot to remove the previous ones, that's what the warning seems to be for

@LOuroboros
Copy link
Contributor

Did you ensure you used a fresh copy of this repo with the changes described here in place? Could be you forgot to remove the previous ones, that's what the warning seems to be for

"Remove the previous ones"? As in remove what exactly? 🤔

Though yes, I did test everything on a clean copy.

Here's a diff: https://pastebin.com/RpXpjaiw

@rawr51919
Copy link
Contributor Author

rawr51919 commented Jun 11, 2022

Did you ensure you used a fresh copy of this repo with the changes described here in place? Could be you forgot to remove the previous ones, that's what the warning seems to be for

"Remove the previous ones"? As in remove what exactly? 🤔

Though yes, I did test everything on a clean copy.

Here's a diff: https://pastebin.com/RpXpjaiw

The instructions didn't quite say to remove the first struct Sprite; definition yet I'll need to test that to make sure that's actually correct or not, seems unclear without tests
Try the fix without removing or try the fix while adding back in the struct Sprite; definition next and see if it works then

@LOuroboros
Copy link
Contributor

Did you ensure you used a fresh copy of this repo with the changes described here in place? Could be you forgot to remove the previous ones, that's what the warning seems to be for

"Remove the previous ones"? As in remove what exactly? 🤔
Though yes, I did test everything on a clean copy.
Here's a diff: pastebin.com/RpXpjaiw

The instructions didn't quite say to remove the first struct Sprite; definition yet I'll need to test that to make sure that's actually correct or not, seems unclear without tests Try the fix without removing or try the fix while adding back in the struct Sprite; definition next and see if it works then

I already told you above though? That without removing it, a ROM cannot get built. The compiler complains about the other struct Sprite that does get used.

@rawr51919
Copy link
Contributor Author

What wizardry's causing it to build perfectly fine without the flicker fix and both struct Sprites yet when applied here it just errors? Sounds like a compile test is in order, will do ASAP

@LOuroboros
Copy link
Contributor

LOuroboros commented Jun 11, 2022

Maybe the first struct Sprite that is completely empty is ignored in favor of the other one located a few lines below which is not empty or something? 🤔

EDIT: Still, I could try to re-apply everything again on a separate, also clean branch, if it's needed.

@rawr51919
Copy link
Contributor Author

Might be a good thing to try, perhaps it's some weirdness with the compiler

@LOuroboros
Copy link
Contributor

LOuroboros commented Jun 12, 2022

Might be a good thing to try, perhaps it's some weirdness with the compiler

Done. Strangely, I'm now getting a different error.

src/item_menu_icons.c: In function `HideBagItemIconSprite':
src/item_menu_icons.c:642: `spriteIds' undeclared (first use in this function)
src/item_menu_icons.c:642: (Each undeclared identifier is reported only once
src/item_menu_icons.c:642: for each function it appears in.)
make: *** [Makefile:337: build/emerald/src/item_menu_icons.o] Error 1
make: *** Deleting file 'build/emerald/src/item_menu_icons.o'
make: *** Waiting for unfinished jobs....

I followed the instructions straight from the document in your branch.

https://github.com/coltongit/pokeemerald/blob/patch-1/docs/bugs_and_glitches.md

Here's the diff: https://pastebin.com/cqNED6Rz

@GriffinRichards
Copy link
Member

Because the changes to HideBagItemIconSprite are nonsense. They're incompletely renaming two separate things: an array of sprite ids in the global struct and a local pointer to one of those sprite ids.

Please test your changes.

@rawr51919
Copy link
Contributor Author

rawr51919 commented Jun 12, 2022

Because the changes to HideBagItemIconSprite are nonsense. They're incompletely renaming two separate things: an array of sprite ids in the global struct and a local pointer to one of those sprite ids.

Please test your changes.

Haven't had time to as of yet, will do so ASAP
EDIT: @GriffinRichards I looked at that piece of code again and it should be OK now, just needs some testing

@rawr51919
Copy link
Contributor Author

@LOuroboros Poked and prodded with a fine-toothed comb and ready for another test

@LOuroboros
Copy link
Contributor

@LOuroboros Poked and prodded with a fine-toothed comb and ready for another test

Tested it, a ROM gets built now, but the issue that was happening before editing the instructions (see #1514) still happens where items' sprites overlap as you select them in the bag.

%pn_20220614_1517209

@rawr51919
Copy link
Contributor Author

rawr51919 commented Jun 16, 2022

@LOuroboros Poked and prodded with a fine-toothed comb and ready for another test

Tested it, a ROM gets built now, but the issue that was happening before editing the instructions (see #1514) still happens where items' sprites overlap as you select them in the bag.

%pn_20220614_1517209

I might need some help with that part of it, yet at least it's working as it was now so that's a start

@rawr51919
Copy link
Contributor Author

@LOuroboros, @ShinyDragonHunter has provided a potential fix for that issue that we both are willing to help you test, try it with this version of the function and see what happens

@LOuroboros
Copy link
Contributor

LOuroboros commented Jun 16, 2022

What SDH probably suggested was to edit the existing RemoveBagItemIconSprite function, not to add it as if it was a new one like your document says, because then the compiler would likely complain about having 2 functions with the exact same name.

I'm already testing it though.

EDIT: Doesn't seem to work.

%pn_20220616_1309202

@ShinyDragonHunter
Copy link
Contributor

It was what I was suggesting, yeah. I do apologise if I wasn't clear on that

@rawr51919
Copy link
Contributor Author

rawr51919 commented Jun 16, 2022

Fix tweaked in the docs, hopefully it's finally working now
Yep, it's fine now!

@rawr51919
Copy link
Contributor Author

@GriffinRichards This PR's ready, everyone in here confirms the fix now works properly

@rawr51919
Copy link
Contributor Author

Because the changes to HideBagItemIconSprite are nonsense. They're incompletely renaming two separate things: an array of sprite ids in the global struct and a local pointer to one of those sprite ids.

Please test your changes.

Not only fixed, the original bug this aimed to fix has been fixed as well.

antoinejen pushed a commit to antoinejen/pokeemerald that referenced this pull request Jul 18, 2022
Item flicker fix now in `BUGFIX`
Supersedes and resolves pret#1680
Fixes pret#1215
Fixes pret#1514
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants