Vanilla Fix - Sell Unit Exploit

[CnCRAZER]

Improvements to sell-target selection:

• Added a hook in src/Misc/Hooks.BugFixes.cpp (ScrollClass_ChooseAction_SellWall) to disallow the sell action on wall overlays if the mouse cursor is hovering over another object, ensuring the correct target is selected.
• Added a hook in src/Misc/Hooks.BugFixes.cpp (HouseClass_SellOverlay_ObjectCheck) to prevent the sell action on wall overlays at the event/network level if there is another object present on the cell.

Supporting changes:

• Included the EventClass.h header in src/Misc/Hooks.BugFixes.cpp to support the new event-level sell overlay logic.

[DeathFishAtEase]

Hey! Don't forget to add a Changelog entry in Whats-New.md, it's basically a must-have.
Also, new entries in Fixed-or-Improved-Logics.md are generally placed at the end of the same category (I noticed you inserted it in the middle).

[github-actions]

Nightly build for this pull request:

• compiled-dll-8a493a1b39d188fd0b24651af524d00e6771f649.zip
  These artifacts will expire in 90 days and will not be available for download after that time.

This comment is automatic and is meant to allow guests to get latest nightly builds for this pull request without registering. It is updated on every successful build.

[fagonghaiwo]

tested, it works👀

[Metadorius]

merged too fast, no changelog and credits entries, also IIRC @Starkku was looking into it too

[DeathFishAtEase]

I have supplemented the Changelog (Whats-New.md). If you (RAZER) would like to add to or improve the documentation, you can submit your changes to the dev-docs branch of my fork :)
(this follows the standard method for making pure documentation changes. Since you cannot directly edit #2108 , you may need to open a PR to my fork to do this).

[Metadorius]

You could probably just add a CREDITS.md entry and it'll be fine. Also IMO such changes could be added directly, since they're basically fixing a critical issue. Doc readability improvements and translation synchronization should be done as you do, of course.

[DeathFishAtEase]

You could probably just add a CREDITS.md entry and it'll be fine. Also IMO such changes could be added directly, since they're basically fixing a critical issue. Doc readability improvements and translation synchronization should be done as you do, of course.

Based on the previous control scheme of the label checker^[1], my understanding is that a bugfix does not require a record in the Fixed-or-Improved-Logics.md document or CREDITS, but a Changelog entry is mandatory. Isn't that the case? According to this rule, only the Changelog is mandatory, while the other two depend on the PR author.

I think the more important issue is how to coordinate between the ongoing issue investigation by Starkku and the fact that this PR has been merged.

---

1. See e2e06c7

[Metadorius]

Isn't that the case?

Not really. What you said applies to Phobos bugfixes (also changelog entry is only required when it's a bugfix to a bug that was found in a released stable version). Vanilla fixes, however, are much more complex and have no reason to be excluded from any of the required docs.

[DeathFishAtEase]

have no reason to be excluded from any of the required docs

What I mean is that CREDITS has always been more of something that PR authors decide to add based on their own discretion, just like how we used to use Minor label (later No Documentation Needed) to allow some PRs whose authors believed no CREDITS (or other documentation changes) were needed to pass workflow checks.

[Starkku]

Just for a note that the fix itself is also buggy, the hook in EventClass::Execute (incorrectly labeled as HouseClass_SellOverlay in the code) is broken on both technical (it is hooked on call instruction) and design (it is intended to block selling of walls if it shares a cell with objects but this was never the issue and whoever wrote it didn't have a clue, should check selling of units outside where you're supposed to instead) levels.

[CnCRAZER]

You could probably just add a CREDITS.md entry and it'll be fine. Also IMO such changes could be added directly, since they're basically fixing a critical issue. Doc readability improvements and translation synchronization should be done as you do, of course.

I'm not too concerned with credits overall. Not like this is some super higly sought after feature. Just a bugfix.

[Metadorius]

I'm not too concerned with credits overall. Not like this is some super higly sought after feature. Just a bugfix.

It's a project policy, so you should credit all the vanilla fixes and feature contributions you make.

[DeathFishAtEase]

It's a project policy, so you should credit all the vanilla fixes and feature contributions you make.

I think the real issue is that we lack a clear classification of which types of contributions the Contributing changes to the project section applies to, and the label checker also needs to assign the corresponding correct skip settings.

[Metadorius]

There are also reports of people crashing when trying to sell walls now.

@Coronia we shouldn't be so fast to merge stuff, and also preferably should check the Discord chat more often as it is still the central gathering point.

[CnCRAZER]

There are also reports of people crashing when trying to sell walls now.

@Coronia we shouldn't be so fast to merge stuff, and also preferably should check the Discord chat more often as it is still the central gathering point.

Can confirm this as well. Just crashed trying to sell walls.

[DeathFishAtEase]

tested, it works👀

Could you list in detail the scenarios you have tested to help us eliminate those already tested?