Conversation
Grantapher
left a comment
There was a problem hiding this comment.
Can you switch your formatter to 4x spaces instead of tabs for this project?
I haven't fully reviewed the Turret patch code yet.
| namespace ValheimPlus.GameClasses | ||
| { | ||
| [HarmonyPatch(typeof(Projectile), nameof(Projectile.IsValidTarget))] | ||
| public static class Projectile_IsValidTarget_Patch |
There was a problem hiding this comment.
Is this a bug fix? If so, is there a ticket submitted already?
There was a problem hiding this comment.
It's a bug fix for something that I noticed while I was playing around with turrets, but the bug that was listed in the Discord was about the different configuration values. I suspect it wasn't working because he said he set it to "shoot 100% faster," which is actually addressed in the configuration changes.
There was a problem hiding this comment.
I mean for the game itself, not the mod.
| } | ||
| public class TurretConfiguration : ServerSyncConfig<TurretConfiguration> | ||
| { | ||
| public bool enablePvP { get; internal set; } = false; |
There was a problem hiding this comment.
Can we keep all the old settings and then add the new ones at the bottom? I don't want to get rid of settings people may currently be using, nor change the order of them in any of the files. For the config specifically, the mod tries to add new config items as they are added in newer mod versions, so having that order would match how it would be added to people's config files.
There was a problem hiding this comment.
Prior to this change "ignorePlayers" would have to be enabled all the time in order for turrets to function normally. This is simultaneously a bug fix and way to give more meaning to this setting.
ValheimPlus/GameClasses/Turret.cs
Outdated
| { | ||
| var config = Configuration.Current.Turret; | ||
| if (!config.IsEnabled) return; | ||
| __instance.m_targetPlayers = config.enablePvP && TurretHelpers.IsCreatorPvP(__instance); |
There was a problem hiding this comment.
Does this injure non-pvp players? It could be used to pvp grief people who are not pvp-enabled if so.
There was a problem hiding this comment.
To add to what I said before in the comment about "ignorePlayers" being changed to "enablePvP," it's exactly this reason that ignorePlayers always had to be set to true. I think it can injure players without PvP so I'm probably going to add a hit detection and only apply damage if the target also has PvP enabled.
There was a problem hiding this comment.
Okay I have confirmed it will not damage the player if they don't also have PvP enabled because of the way setting a projectile's "m_owner" works. The RPC_Damage call on Character class will only allow the projectile damage through if the character being damaged is a player with PvP enabled. However, turrets might still target non-PvP players, so I'm going to check that out.
|
Vanilla turrets' AI targets ANYTHING. This mod sought to solve the major bug by adding the "ignorePlayers" setting. I believe we should set the default behavior of the turrets to only allow them to target players when the creator has PvP Enabled, unless otherwise specified in the config by changing "disablePvP." This solves the bug of always targeting players, and still allows the functionality when "disablePvP" is set to false. However, when the creator enables PvP the targeting bug still persists as it will target ANY other player. To fix this I will patch the turrets so that it will only find targets whose PvP is also enabled. The compromise in this case is changing "ignorePlayers" to "disablePvP." I previously had it set to "enablePvP" but since majority of players using Turrets would have turned on "ignorePlayers" this seems like the opposite of what would be intended. By changing it to "disablePvP" functionality will continue as expected, and if players desire to allow turrets in PvP they can update the setting. Additionally, people who fresh install the mod will also have turrets fixed. They will be functional in PvP until "disablePvP" is set to true, but I think that should be the expected behavior of vanilla turrets. |
Grantapher
left a comment
There was a problem hiding this comment.
I think I'm missing something here when it comes to turn rate vs projectile accuracy and projectile velocity...
| verticalAngle = 50 | ||
|
|
||
| ; Set this to true to prevent the turret from accidentally hitting itself. | ||
| fixProjectiles = false |
There was a problem hiding this comment.
Let's make the name precise, turretCanNotHitItself.
| var turretConfig = Configuration.Current.Turret; | ||
| if (!turretConfig.IsEnabled || !turretConfig.fixProjectiles) return; | ||
|
|
||
| if (__result && destr is Turret turret) |
There was a problem hiding this comment.
My IDE flags this saying that Turret isn't an IDestructible, are you sure this works? Is it supposed to be like a .GetComponent<IDestructible>() on some GameObject? If your log is going off then I guess this is working but I would not know how.
| public float turnRate { get; internal set; } = 22.5f; | ||
| public float attackCooldown { get; internal set; } = 2f; | ||
| public float viewDistance { get; internal set; } = 45f; |
There was a problem hiding this comment.
We can't change how these are calculated, all currently existing values would be invalid and do wild things. Stick to the modifiers.
We also don't want to use the game's values in case the devs tweak them, because then we would have to keep our values up to date with theirs.
| OpCodes.Brtrue, | ||
| new(inst => inst.IsLdarg(11)), | ||
| OpCodes.Brfalse | ||
| ).ThrowIfNotMatch("Could not find `onlyTargets` in BaseAI.FindClosestCreature transpiler") |
There was a problem hiding this comment.
Make sure these throws are wrapped so they don't propagate and break the entire mod. See other places where CodeMatcher is used for an example.
| namespace ValheimPlus.GameClasses | ||
| { | ||
| [HarmonyPatch(typeof(Projectile), nameof(Projectile.IsValidTarget))] | ||
| public static class Projectile_IsValidTarget_Patch |
There was a problem hiding this comment.
I mean for the game itself, not the mod.
| var turretOwner = TurretHelpers.GetPlayerCreator(turret); | ||
| if (turretOwner == __instance.m_owner) | ||
| { | ||
| ValheimPlusPlugin.Logger.LogInfo("Turret projectile hit itself"); |
There was a problem hiding this comment.
Let's remove log statements like this if you are confident it is working.
| public float projectileVelocity { get; internal set; } = 0f; | ||
| public float projectileAccuracy { get; internal set; } = 0f; |
| } | ||
| } | ||
| } | ||
| "ValheimPlus": { |
There was a problem hiding this comment.
Looks like white space changes hit this file. Can you undo the white space changes here?
| public float turnRate { get; internal set; } = 22.5f; | ||
| public float attackCooldown { get; internal set; } = 2f; | ||
| public float viewDistance { get; internal set; } = 45f; | ||
| public bool targetTamed { get; internal set; } = false; |
There was a problem hiding this comment.
I believe this should be true in the vanilla game
| public class TurretConfiguration : ServerSyncConfig<TurretConfiguration> | ||
| { | ||
| public bool ignorePlayers { get; internal set; } = false; | ||
| public bool disablePvP { get; internal set; } = false; |
There was a problem hiding this comment.
unfortunately I don't think we can get rid of an old config like this. Can we keep ignorePlayers and also add the new disablePvP config?
Updated the config, added and removed some settings, generally made it more user-friendly.