after `@pvpoff` the cursor stay as sword icon, on rathena it doesn't

[AnnieRuru]

Describe the bug on hercules, after @pvpoff the cursor still stay as sword icon on rathena, after @pvpoff the cursor returns to normal select icon

Screenshots

To Reproduce

1. @pvpon
2. @pvpoff

the sword cursor still stay

Expected behavior It should works like rathena's on rathena, after @pvpoff the attack sword cursor becomes a normal select cursor

System specs (please complete the following information):

• OS: Windows 10 pro 64-bit
• Hercules Version: latest (always pull latest git)
• Packet version: 20190530
• Mode: Renewal
• Client type: 2019-05-30aRagexeRE
• Branch: Main

Plugins used or source modifications none, purposely unload all plugins, tested on both latest clean rathena and clean hercules

Additional context no wonder I couldn't get my cell_pvp plugin to work initially I think there is a bug with map->zone_change2 function rathena clean up their map_property function before

also if anyone reproduce this, this issue is extremely minor <-- just a visual bug

[Kenpachi2k13]

Confirmed! It seems that clif_maptypeproperty2() is buggy. I implemented rAthena's code in clif_map_property_mapall() for testing:

#if PACKETVER >= 20121010
	struct map_data *mapdata = &map->list[bl.m];

	WBUFL(buf,4) = ((mapdata->flag.pvp?1:0 || (bl.type == BL_PC && ((TBL_PC *)&bl)->duel_group > 0))<<0)| // PARTY - Show attack cursor on non-party members (PvP)
		((mapdata->flag.battleground || map_flag_gvg2(bl.m)?1:0)<<1)|// GUILD - Show attack cursor on non-guild members (GvG)
		((mapdata->flag.battleground || map_flag_gvg2(bl.m)?1:0)<<2)|// SIEGE - Show emblem over characters heads when in GvG (WoE castle)
		((0 || !map_flag_gvg2(bl.m)?0:1)<<3)| // USE_SIMPLE_EFFECT - Automatically enable /mineffect
		((0 || map_flag_vs(bl.m)?1:0)<<4)| // DISABLE_LOCKON - Only allow attacks on other players with shift key or /ns active
		((mapdata->flag.pvp?1:0)<<5)| // COUNT_PK - Show the PvP counter
		((mapdata->flag.partylock?1:0)<<6)| // NO_PARTY_FORMATION - Prevents party creation/modification (Might be used for instance dungeons)
		((mapdata->flag.battleground?1:0)<<7)| // BATTLEFIELD - Unknown (Does something for battlegrounds areas)
		(((mapdata->flag.noviewid & EQP_COSTUME)?1:0)<<8)| // DISABLE_COSTUMEITEM - Disable costume sprites
		((1)<<9)| // USECART - Allow opening cart inventory (Well force it to always allow it)
		((0)<<10); // SUNMOONSTAR_MIRACLE - Blocks Star Gladiator's Miracle from activating
		//(1<<11); // Unused bits. 1 - 10 is 0x1 length and 11 is 0x15 length. May be used for future settings.
#endif

And this works as intended. I have to investigate this further...

[Kenpachi2k13]

Okay... I tracked down the culprit.

We're always sending the the old 0x199 (ZC_NOTIFY_MAPPROPERTY) prior to sending the new 0x99b (ZC_MAPPROPERTY_R2). (There is one exception in clif_parse_LoadEndAck() where 0x199 isn't sent every time 0x99b is send.)

Especially when activating/deactivating PK flags (PvP/GvG/CvC) this seems to confuse the client which is reasonable when checking the processing order:

• Activate PvP by using @pvpon.
  • Send 0x199 which only sets the map's property to MAPPROPERTY_FREEPVPZONE.
  • Send 0x99b which:
    • Sets the map's type to 0x28 (No idea why Ind chose this value. Actually it should be a MAPTYPE_* constant. But funnily this doesn't have an effect in this use case.).
    • Sets the map's flags. (See struct packet_maptypeproperty2 in src/map/packets_struct.h for details.)
• Deactivate PvP by using @pvpoff.
  • Send 0x199 which only sets the map's property to MAPPROPERTY_NOTHING.
  • Send 0x99b which:
    • Sets the map's type to 0x28 again.
    • Sets the map's flags. (See struct packet_maptypeproperty2 in src/map/packets_struct.h for details.)

Needless to say that this doesn't make any sense, huh?

In my opinion we should:

1. Adjust struct packet_maptypeproperty2 to be usable for both 0x199 and 0x99b. (And probably rename it to packet_maptypeproperty.)
2. Move the related code to handle the struct to clif_map_property() (also change paramter sd to bl and add parameter for send target) and get rid of clif_maptypeproperty2().
3. Adjust clif_map_property_mapall() to simply redirect to clif_map_property() with a dummy bl and send target ALL_SAMEMAP.

This way the code would be centralised and sending both packets becomes impossible.

I hope my explanation is kinda understandable and not too confused.

@4144 @Asheraf @hemagx @MishimaHaruna @skyleo What do you think?