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

Apply new binary patch to Salem #6499

Merged
merged 5 commits into from
Nov 7, 2024
Merged

Conversation

4z0t
Copy link
Member

@4z0t 4z0t commented Oct 30, 2024

Salem destroyer has special ability to move from water onto land. However in vanilla game this feature sometimes may cause problems with positioning of the unit and it's pathfinding. FAF introduced solution with a binary patch which makes it toggable ability to walk on land.
But this solution had flaws:

  1. works only for salem: you can't make it work for other unit (mod one for example);
  2. solution is unsafe: you can't restart game or replay because if any salem walks it just causes a crash;
  3. solution uses weird way to do that toggle: it confuses doing this way;
  4. this breaks filtering based on blueprint id.

With new assembly patch these flaws are no more. This PR is about applying new assmbly fix.

Description of the proposed changes

Applies patch changes and removes old ones.

Testing done on the proposed changes

  1. This patch can be applied to any other unit to achive same behavior (no example provided)
  2. Start skirmish match. Spawn salem and make it walk. Restart match (not closing game). Spawn salem again and make it walk. Game works with no problems.
  3. Function description isn't great since it relies on internal work of the game, but still it isn't done with GetStat.
  4. Spawn 2 salems, make one of them go to land. Select one of them, press ctrl-z (select similar units) or with cltr-left-click, second one is selected too.

Additional context

This change requires an asm patch.

Checklist

  • Changes are annotated, including comments where useful
  • Changes are documented in the changelog for the next game version

@4z0t 4z0t changed the title apply salem toggle fix Salem toggle fix v2.0 Oct 30, 2024
@4z0t 4z0t changed the title Salem toggle fix v2.0 Apply new binary patch to Salem Oct 30, 2024
@4z0t 4z0t added area: requires engine patch related to the engine and no effective workaround is known in Lua area: sim Area that is affected by the Simulation of the Game labels Oct 30, 2024
@4z0t 4z0t requested review from Hdt80bro, Garanas and BlackYps October 30, 2024 17:04
@@ -0,0 +1,23 @@
- (#6499) Apply new binary patch to Salem

Salem destroyer has special ability to move from water onto land. However in vanilla game this feature sometimes may cause problems with positioning of the unit and it's pathfinding. FAF introduced solution with a binary patch which makes it toggable ability to walk on land.
Copy link
Member

@Garanas Garanas Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not copy-paste the pull request description into the changelog snippet. They do not share the same audience. For a pull request description your audience is other programmers and/or contributors who (soon) have a decent idea of how things work. For the changelog the audience is your average joe, they won't understand all the technical details and they don't have to.

I think you can simplify this to:

- (#6499) Fix hard crash when exiting the game when a Salem is on land

The Cybran Tech 2 Destroyer (Salem) has the special ability to move from water onto land. There was a bug in a special, alternative implementation introduced by FAForever. Together with an assembly patch this bug is now fixed.

Interested in how assembly patches work? Reach out to the game team. You can find us easiest via the official Discord server.

The above would be of category 'fix'. The below would be of category 'feature':

- (#6499) Enable dynamic footprint changes for (modded) units

With thanks to an assembly patch the footprint changes that were previously unique to the Cybran Tech 2 Destroyer can now be applied to any unit. For more details, see the pull request on GitHub. 

Interested in how assembly patches work? Reach out to the game team. You can find us easiest via the official Discord server.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not correct to call it dynamic footprint change. Because internally game still uses both of them based on motion type (if both are provided). Here we are just forcing to use only one in particular for water.

Copy link
Member

@Garanas Garanas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general. @BlackYps do we want to include this in the release? One less hard crash to desktop is always nice.

@BlackYps
Copy link
Contributor

BlackYps commented Nov 5, 2024

Yes, this sounds like a good addition

@Garanas Garanas merged commit c69792c into FAForever:develop Nov 7, 2024
5 checks passed
Garanas pushed a commit to FAForever/FA-Binary-Patches that referenced this pull request Nov 7, 2024
@Garanas Garanas mentioned this pull request Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: requires engine patch related to the engine and no effective workaround is known in Lua area: sim Area that is affected by the Simulation of the Game
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants