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

Changes to make adding scopes possible #14257

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jade-Harleyy
Copy link

@Jade-Harleyy Jade-Harleyy commented Jul 5, 2024

This PR does two things:

  1. Adds a new attribute to the Holdable component: CameraAimOffset.
  2. Reworks vision obstruction to work with long range sight.

Mod for Testing: Mod.zip

Obstruction Rework Previews

Lighting Disabled
image
Lighting Disabled w/ Scope
image
Lighting Enabled
image
Lighting Enabled w/ Scope
image

Reworked Light Sprites

Content/Lights/lightcone.png
Content/Lights/visioncircle.png
Content/Lights/visioncircle.png
Content/Lights/visioncircle.png

Copy link
Collaborator

@Regalis11 Regalis11 left a comment

Choose a reason for hiding this comment

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

Found some code quality issues. It's also unclear to me why and how exactly the behavior of the vision circle logic was changed.

@@ -425,6 +425,10 @@ void ResetInputIfPrimaryMouse(InputType inputType)
{
cam.OffsetAmount = targetOffsetAmount = item.Prefab.OffsetOnSelected * item.OffsetOnSelectedMultiplier;
}
else if (HeldItems.FirstOrDefault(item => item.GetComponent<Holdable>()?.Aimable ?? false) is Item item2 && IsKeyDown(InputType.Aim))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pattern matching should be used instead of null coalescing here. The names item and item2 are not very descriptive, and they're quite misleading because they're in this case the same item.

@@ -425,6 +425,10 @@ void ResetInputIfPrimaryMouse(InputType inputType)
{
cam.OffsetAmount = targetOffsetAmount = item.Prefab.OffsetOnSelected * item.OffsetOnSelectedMultiplier;
}
else if (HeldItems.FirstOrDefault(item => item.GetComponent<Holdable>()?.Aimable ?? false) is Item item2 && IsKeyDown(InputType.Aim))
{
cam.OffsetAmount = targetOffsetAmount = item2.GetComponent<Holdable>().CameraAimOffset;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fetching the Holdable component is done twice

float scale = (1f - ObstructVisionAmount) * scaleMultiplier;
Vector2 circleOrigin = new Vector2(visionCircle.Width, visionCircle.Height) / 2f;
Vector2 coneOrigin = new Vector2(0f, visionCone.Height / 2f);
Vector2 coneScale = new Vector2(1f / visionCone.Width, scale / visionCone.Height * coneHeightMultiplier) * (losOffset.Length() + circleOrigin.X);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to change the behavior of the existing vision circle too. I think this needs some clarification: how and why does it change the vision circle too?

@Regalis11 Regalis11 added Feature request Request a new feature to be added Code Programming task Modding Modding-related feature request or issue, or a bug that only occurs with mods Waiting Cannot be worked on/reviewed/tested at the moment for some reason labels Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Programming task Feature request Request a new feature to be added Modding Modding-related feature request or issue, or a bug that only occurs with mods Waiting Cannot be worked on/reviewed/tested at the moment for some reason
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants