(Default Off) Item Description Headers

[ghoulslash]

Adds description headers from my pokeemerald branch. It uses callnative instead of script cmds to cause fewer conflicts. A config has been added which is Off by default so as not to break saves. I added the itemFlags to saveblock3 as well

config:
0 = off
1 = on first obtain (uses save space)
2 = always shot description (no save space required)

[AlexOn1ine]

Ghoul, do you think you could add an option that shows the header without using up saveblock space so it will basically show up each time you obtain the item even if it is a duplicate?

[Bassoonian]

Do you think there’s a way this could depart from your actual branch? It’s a commonly merged in branch, so if expansion can handle the conflicts from the original to this one, it would make it significantly easier for people downstream

[ghoulslash]

Ghoul, do you think you could add an option that shows the header without using up saveblock space so it will basically show up each time you obtain the item even if it is a duplicate?

Yeah, that's a good option. OW_SHOW_ITEM_DESCRIPTIONS = 0: off, 1 = first time, 2 = always ?

Do you think there’s a way this could depart from your actual branch? It’s a commonly merged in branch, so if expansion can handle the conflicts from the original to this one, it would make it significantly easier for people downstream

Yeah, can probably move out of item.c to help conflicts. Good idea.

[AlexOn1ine]

Yeah, that's a good option. OW_SHOW_ITEM_DESCRIPTIONS = 0: off, 1 = first time, 2 = always ?
Sounds good, thx.

[ghoulslash]

okay added extra config values and moved code to overworld.c. ready for re review

[AlexOn1ine]

why is there an unused?

[ghoulslash]

why is there an unused?

For when the config is off. Otherwise you end with a bunch of warnings

[Bassoonian]

I just tried compiling it with line 3478 removed (making it an empty function like the one below), and it compiles just fine :)

[mrgriffin]

I think the ScriptReadByte(ctx); is necessary to consume the argument of showitemdescription and showberrydescription (i.e. .byte 0 and .byte 1 respectively). Otherwise those bytes will be interpreted as commands 0 and 1 (nop and nop1 respectively, which are harmless in Expansion, but ripe for being replaced by downstream projects).

[Bassoonian]

Then perhaps we need to just add the ScriptReadByte functions but without specified variable to store the returned value in?

[AlexOn1ine]

@mrgriffin, @ghoulslash could any of you respond to this please? 👀

[AlexOn1ine]

@mrgriffin @ghoulslash could any of you get back to this? It is the last thing needed to be addressed for a merge 😅

[AlexOn1ine]

Is there a way we could block the contents of GetSetItemObtained and saveblock with an ifdef block and the rest with a normal if condition to get rid of the ifdefs in the scripts?

[pkmnsnfrn]

If we can complete an emerald playthrough with this on, would that be enough to say "it's probably good"?

[AlexOn1ine]

Conflicts

[ghoulslash]

conflicts handled and removed saveblock address comment from global.h. Ready for re-review
If we can complete an emerald playthrough with this on, would that be enough to say "it's probably good"?
that would be the idea scenario, but it's a pretty big requirement. I think the branch has existed long enough that most test cases are handled by now

[pkmnsnfrn]

that would be the idea scenario, but it's a pretty big requirement

its 3 hours to get to the champion at 1x. I'm just curious what you and alex think is an acceptable merge criteria after the code review, and it sounds like you're saying "nothing, this PR is already battle tested"

[AlexOn1ine]

that would be the idea scenario, but it's a pretty big requirement

its 3 hours to get to the champion at 1x. I'm just curious what you and alex think is an acceptable merge criteria after the code review, and it sounds like you're saying "nothing, this PR is already battle tested"

A play-through is not necessary. I'll merge it once the open question gets answered.

[Bassoonian]

In my old implementation in my hack, I had a memset(&gSaveBlock2Ptr->itemFlags, 0, sizeof(gSaveBlock2Ptr->itemFlags)); in NewGameInitData. This doesn't seem to be here, which means that the item flags don't get reset. Is this intentional?

move code to overworld.c to prevent issues with users whove merged original branch

This also failed 100% because the original code doesn't get moved, it just gets duplicated right now which leaves behind a bunch of silent conflicts for the user to solve

[AlexOn1ine]

I squashed this PR because ghoul made a brand new branch that had nothing to do with the old history

[AsparagusEduardo]

I squashed this PR because ghoul made a brand new branch that had nothing to do with the old history

We should do the same thing we did for BW map headers then, so avoid conflicts for downstream.