Skip to content

Conversation

@MadaraUchiha
Copy link
Contributor

@MadaraUchiha MadaraUchiha commented Apr 15, 2020

Additions

N/A

Changes

Refactors turret reload mechanics

Breaking Changes:

  • Reloading turrets directly from inventory is not possible anymore. Players need to make the pawn drop from inventory first.

    This behavior is consistent with vanilla refueling.

References

CombatExtendedRWMod#1212

Reasoning

Removing the GenClosestAmmo class as suggested by maintainers on CombatExtendedRWMod#1209.

Alternatives

N/A

Testing

Check tests you have performed:

  • Compiles without warnings
  • Game runs without errors
  • Playtested a colony (3 hours)

@MadaraUchiha MadaraUchiha requested review from a team April 15, 2020 20:16
Copy link
Contributor

@N7Huntsman N7Huntsman left a comment

Choose a reason for hiding this comment

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

This is a test review.

@N7Huntsman N7Huntsman requested review from a team and N7Huntsman and removed request for N7Huntsman April 15, 2020 20:43
@N7Huntsman N7Huntsman dismissed their stale review April 15, 2020 20:45

Testing purposes.

Copy link
Contributor

@N7Huntsman N7Huntsman left a comment

Choose a reason for hiding this comment

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

I get the following error when manually order a pawn to reload (through the UI gizmo) a manned turret they are operating when there is none of that kind of ammo available. https://pastebin.com/ntAAG07F

I get this error when the turret runs out of ammo and the pawn attempts to reload automatically and there is none of the kind of ammo they were using available. https://pastebin.com/E6vYB68U

@N7Huntsman
Copy link
Contributor

N7Huntsman commented Apr 18, 2020

The errors associated with manned turrets becoming "attached" to stacks of ammo no longer seems to be occurring, and operators are now swapping between ammo types as intended and will travel through doors to fetch ammo. Progress!

Reloading turrets directly from inventory is not possible anymore. Players need to make the pawn drop from inventory first.

Out of curiosity, is there any intention to restore this functionality? And will colonists automatically drop ammo from their inventory and use it if it's the only available, or does the player have to do it manually?

N7Huntsman
N7Huntsman previously approved these changes Apr 18, 2020
@N7Huntsman
Copy link
Contributor

N7Huntsman commented Apr 18, 2020

@MadaraUchiha Haven't run into any issues during testing, and the error messages are gone. I'd like to get some people with saves affected by the turret issue to load this version of CE to see what it does, but otherwise I'm happy to green-light this.

Would it be feasible for colonists reloading turrets to check in the immediate vicinity (think a 3 or 4 tile radius) of the ammo stack they're grabbing if there's not enough there to completely reload the turret?

Currently, if they're reloading a turret with FMJ that has an empty magazine and a capacity of 100 rounds, and there's a stack of 75 FMJ and a stack of 40 FMJ right next to each other, the colonist will just grab the stack of 75 and partially reload the turret.

While there's no sense sending colonists across the map for the last 2 rounds to top off the magazine, would it be feasible to make them grab from another nearby stack to completely fill the turret magazine? If it'd be a big headache of new checks and jobs and such don't worry about it (since the "solution" is to just order your pawn to reload again), but it might be a nice QoL thing.

@MadaraUchiha
Copy link
Contributor Author

@MadaraUchiha Haven't run into any issues during testing, and the error messages are gone. I'd like to get some people with saves affected by the turret issue to load this version of CE to see what it does, but otherwise I'm happy to green-light this.

Would it be feasible for colonists reloading turrets to check in the immediate vicinity (think a 3 or 4 tile radius) of the ammo stack they're grabbing if there's not enough there to completely reload the turret?

Currently, if they're reloading a turret with FMJ that has an empty magazine and a capacity of 100 100 rounds, and there's a stack of 75 FMJ and a stack of 40 FMJ right next to each other, the colonist will just grab the stack of 75 and partially reload the turret.

While there's no sense sending colonists across the map for the last 2 rounds to top off the magazine, would it be feasible to make them grab from another nearby stack to completely fill the turret magazine? If it'd be a big headache of new checks and jobs and such don't worry about it (since the "solution" is to just order your pawn to reload again), but it might be a nice QoL thing.

@N7Huntsman I've considered that. What you're talking about is the difference between the vanilla's Refuel and AtomicRefuel. What we want is sort of a combination of the two.

AtomicRefuel has the capability of reserving and collecting from multiple stacks to fill the requirement to 100%, but it will bail if it can't reach 100%, which is not what we want (or do we?). i.e. if the turret wants 100 rounds, and the player only has 75 in range, nobody would reload.

What I think we want is a combination of the two, try to reserve as much as possible up to the limit, but if you don't find enough ammo to fill it all the way, go through with it anyway.

The AtomicRefuel toils are significantly more complex than the regular Refuel which I've drawn from, and involve labels and Tynan-implemented goto. I gave it a try at first, and I couldn't get it to work properly, and had trouble debugging what went wrong with it.

I do intent to try again, because it's much better UX, but I figured releasing a fix to a game breaking bug has priority over something that can always be added after the fact.

@N7Huntsman N7Huntsman changed the base branch from master to Development April 18, 2020 21:02
@N7Huntsman N7Huntsman changed the base branch from Development to master April 18, 2020 21:02
@N7Huntsman N7Huntsman dismissed their stale review April 18, 2020 21:02

The base branch was changed.

@N7Huntsman N7Huntsman changed the base branch from master to Development April 18, 2020 21:09
Copy link
Contributor

@N7Huntsman N7Huntsman left a comment

Choose a reason for hiding this comment

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

Approved once again.

@MadaraUchiha MadaraUchiha force-pushed the refactor/destroy-genclosestammo-cache-class branch from bf63532 to c751c96 Compare April 18, 2020 22:18
@MadaraUchiha MadaraUchiha changed the title Refactor/destroy genclosestammo cache class Fix Turret Reload Bug Apr 18, 2020
@N7Huntsman
Copy link
Contributor

AtomicRefuel has the capability of reserving and collecting from multiple stacks to fill the requirement to 100%, but it will bail if it can't reach 100%, which is not what we want (or do we?). i.e. if the turret wants 100 rounds, and the player only has 75 in range, nobody would reload.

A partial reload is definitely better than not reloading.

I do intent to try again, because it's much better UX, but I figured releasing a fix to a game breaking bug has priority over something that can always be added after the fact.

Yeah, resolving the issue is certainly the priority. I was just curious if it was something that was on your radar--if we want to just stick to the board as a QoL change for later, that's perfectly fine.

@N7Huntsman
Copy link
Contributor

N7Huntsman commented Apr 19, 2020

Encountered an error while testing mortars: https://pastebin.com/KmgwfBWS

Occurred while trying to swap a loaded HE shell out for an Incendiary shell while in the midst of firing at an assigned target. After that error, I was no longer able to reload the mortar with HE or Incendiary shells. I was, however, able to load and fire EMP shells. When I tried to swap from EMP back to Incendiary, it threw the same error, and now I could no longer load any of the three kinds of ammo.

@N7Huntsman N7Huntsman self-requested a review April 19, 2020 01:37
- Introduce CELogger class to log extra stuff in dev mode.
-- It is recommended to use this class instead of RimWorld's own Log
class.
- Introduce the JobGiverUtils_Reload class
-- Holds interesting job creation and ammo lookup logic.
- Various small refactors
- Small refactors in CompAmmoUser and JobDriver_ReloadTurret.
- Mechanoids now reload turrets with the JobGiver_DefenderReloadTurret
thinknode, patched into the Defend DutyDef.
- Turrets now only order reloads directly for manned pawns (i.e.
mortars)
- Ask for number of rounds missing to full, or the number of rounds in
the found stack. Whichever is smaller.
Will now toss a more meaningful error when MakeReloadJob is called
without checking for CanReload first.
@MadaraUchiha MadaraUchiha force-pushed the refactor/destroy-genclosestammo-cache-class branch from c751c96 to e8e9455 Compare April 19, 2020 15:58
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

@N7Huntsman N7Huntsman requested review from a user April 20, 2020 23:37
@N7Huntsman N7Huntsman merged commit 893774e into CombatExtended-Continued:Development Apr 20, 2020
N7Huntsman added a commit that referenced this pull request Apr 21, 2020
N7Huntsman pushed a commit that referenced this pull request May 12, 2020
VWE Fire Extinguisher Ammo Recipes adjustments
N7Huntsman pushed a commit that referenced this pull request Dec 7, 2020
N7Huntsman pushed a commit that referenced this pull request Dec 24, 2020
N7Huntsman pushed a commit that referenced this pull request Jan 1, 2021
N7Huntsman added a commit that referenced this pull request May 17, 2021
N7Huntsman pushed a commit that referenced this pull request May 17, 2021
Finalize Italian weapon stats
N7Huntsman pushed a commit that referenced this pull request Jan 4, 2022
ghost pushed a commit that referenced this pull request Jan 16, 2022
Tostov added a commit that referenced this pull request Apr 18, 2022
@SamaelGray SamaelGray mentioned this pull request Sep 7, 2022
2 tasks
SamaelGray pushed a commit that referenced this pull request Dec 25, 2022
…nger-apparel-fixed

Minor Patches and Fixes
SamaelGray pushed a commit that referenced this pull request Feb 16, 2023
N7Huntsman pushed a commit that referenced this pull request Jun 19, 2023
SamaelGray pushed a commit that referenced this pull request Jul 7, 2023
@Darknight143 Darknight143 mentioned this pull request Jul 14, 2023
SaltyKarl referenced this pull request in SaltyKarl/CombatExtended Dec 29, 2023
…BlasterRetexture

Charge blaster turret retexture
N7Huntsman pushed a commit that referenced this pull request Apr 4, 2024
N7Huntsman pushed a commit that referenced this pull request Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants