Issue with shoot action checks - did I miss something?

Unity Courses Talk
,
#1

By the end of this module, I had a project that behaves pretty much the same as the one in the videos, but I noticed one pretty bad defect - the shoot action would only highlight targets which were within range and line-of-sight, but if I clicked on an enemy target further away or behind a wall it would perfectly cheerily take the shot and reduce the health of that enemy anyway.

This was because the IsValidActionGridPosition method was only checking whether the target space contained a unit which was an enemy, not whether or not all the other criteria for a valid shot had been met; the constraints on range and line-of-sight had only been implemented in the GetValidActionGridPositionList method. So the enemy units didn’t take advantage of this because they were deriving their valid targets from the GetValidActionGridPositionList, but this is only used for display when taking player actions and the player can just click anywhere to instruct the game to attempt the action on that space.

I fixed the problem pretty easily by refactoring all the validity checks into a new overload of IsValidActionGridPosition that took two params (the source and the target positions) and calling that both from within GetValidActionGridPositionList to filter the valid spaces and also from the regular override of the base class’ IsValidActionGridPosition, which passed in the action’s owning unit’s position for the first param.

Did I miss somewhere where this issue had been returned to and solved in an earlier video? Or perhaps have I introduced this issue with some other changes somewhere else that I’ve not noticed? I’m coming at this course as a professional C# dev of twenty years who just doesn’t know much Unity, so there’s been a lot of points over the course of the videos where I’ve seen ways to do things I think are better; I’ve tried to just follow the course for the most part as generally these issues have been picked up later in the video or refactored out in a subsequent video and I do understand why the most simple approach is taken at first. But I’m still not sure why the GetValidActionGridPositionList implementations didn’t just offload all the validity checks into IsValidActionGridPosition in the first place!

1 Like
#2

Definitely think you missed something. My own code (which is highly modified) as well as the repo shows the GetValidActionGridPosition get’s a list of positions from GetValidActionGridPositionList and then checks if the selected position exists in that list. GetValidActionGridPositionList does all the checks required, including the range, for the selected action.

From the repo

image
image741Ă—114 12.9 KB

1 Like
#3

I can confirm, I just tested trying to shoot an enemy out of range on the Repo version and it wouldn’t shoot. Since the BaseAction.IsValidActionGridPosition tests for valid locations, this should stop the action from occurring in UnitActionSystem.HandleSelectedAction()

    private void HandleSelectedAction()
    {
        if (InputManager.Instance.IsMouseButtonDownThisFrame())
        {
            GridPosition mouseGridPosition = LevelGrid.Instance.GetGridPosition(MouseWorld.GetPosition());

            if (!selectedAction.IsValidActionGridPosition(mouseGridPosition))
            {
                //This should prevent the problem
                return;
            }

            if (!selectedUnit.TrySpendActionPointsToTakeAction(selectedAction))
            {
                return;
            }

            SetBusy();
            selectedAction.TakeAction(mouseGridPosition, ClearBusy);

            OnActionStarted?.Invoke(this, EventArgs.Empty);
        }
    }
1 Like
#4

Cheers - the issue I had wasn’t whether or not it’s calling IsValidActionGridPosition, but rather the implementation of that method; mine was still just checking whether the target space contained an enemy, not whether it was in the list returned by GetValidActionGridPositionList. I must have missed a step at some point previous where the initial implementation got improved upon.

That does still seem completely the wrong way around to me, though - why waste time generating the whole valid list to check an individual space, rather than putting all the logic to check whether a space is valid in the method called “IsValidActionGridPosition” and calling that from GetValidActionGridPositionList to filter the working subset? Is there actually a good reason to do it this way around, or is it just an odd left-over artefact of the MVP approach that each lesson has been taking and building upon bit by bit?

#5

I think if I were refactoring this section for efficiency, I would cache the valid actions when the action is selected, and then referring to that list for methods like IsValidGridPosition().

This is often an artifact of building a system from the ground up as we do in most of our courses. There are often some significant optimizations that can be made to any course project.

1 Like

Privacy & Terms