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

Modify teleport to move vehicle #2111

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

MistaOmega
Copy link
Contributor

Attempting to modify the vehicle teleport behaviour.

Correctly updates player pos with player in seamoth / prawn

@tornac1234 tornac1234 added Area: vehicles Related to vehicles (seamoth, cyclops, prawn) Type: enhancement Issue is to be solved by implementing new features labels Jan 21, 2024
}
catch (Exception e)
{
// Freeze the player while he's loading its new position
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Freeze the player while he's loading its new position


Vehicle currentVehicle = Player.main.currentMountedVehicle;
// Check to make sure the player is in a vehicle
if (currentVehicle != null)
Copy link
Member

Choose a reason for hiding this comment

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

It's heavily advised to not check unity components and gameobjects against null. Instead just handle the object as a bool.

Suggested change
if (currentVehicle != null)
if (currentVehicle)

See: https://discussions.unity.com/t/how-can-i-check-if-an-object-is-null/23224/4

public static bool Prefix(Vector3 position, bool gotoImmediate = false)
{
Vehicle currentMountedVehicle = Player.main.currentMountedVehicle;
if (currentMountedVehicle == null)
Copy link
Member

Choose a reason for hiding this comment

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

Same here with checking against null

Suggested change
if (currentMountedVehicle == null)
if (!currentMountedVehicle)


public sealed partial class GoTo_Patch : NitroxPatch, IDynamicPatch
{
public static readonly MethodInfo TARGET_METHOD = Reflect.Method((GotoConsoleCommand gotoConsoleCommand) => gotoConsoleCommand.GotoPosition(default, default));
Copy link
Member

Choose a reason for hiding this comment

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

Minor, just bc other patched have it this way:

Suggested change
public static readonly MethodInfo TARGET_METHOD = Reflect.Method((GotoConsoleCommand gotoConsoleCommand) => gotoConsoleCommand.GotoPosition(default, default));
public static readonly MethodInfo TARGET_METHOD = Reflect.Method((GotoConsoleCommand t) => t.GotoPosition(default, default));

@@ -0,0 +1,22 @@
using System.Reflection;
using HarmonyLib;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
using HarmonyLib;


namespace NitroxPatcher.Patches.Dynamic;

public sealed partial class GoTo_Patch : NitroxPatch, IDynamicPatch
Copy link
Member

Choose a reason for hiding this comment

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

Patches are named like: <PatchedClass>_<PatchedMethod>_Patch.cs. Could you rename the file and class in this fashion?

@MistaOmega MistaOmega requested a review from Jannify May 24, 2024 15:00
@Measurity Measurity added this to the 1.8 milestone Aug 9, 2024
{
public static readonly MethodInfo TARGET_METHOD = Reflect.Method((GotoConsoleCommand t) => t.GotoPosition(default, default));

public static bool Prefix(Vector3 position, bool gotoImmediate = false)
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to include the parameters if you don't need them.

Suggested change
public static bool Prefix(Vector3 position, bool gotoImmediate = false)
public static bool Prefix(Vector3 position)

Copy link
Member

@Jannify Jannify left a comment

Choose a reason for hiding this comment

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

Except the one comment LGTM CW

Copy link
Collaborator

@tornac1234 tornac1234 left a comment

Choose a reason for hiding this comment

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

This PR is lacking a feature which is allowing the warpforward command to also work with vehicles as well

Also please apply a rebase to this branch so you can test it with the latest movement PR merged

@MistaOmega MistaOmega force-pushed the Modify-Teleport-To-Move-Vehicle branch from 79b87b0 to 495287f Compare November 22, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: vehicles Related to vehicles (seamoth, cyclops, prawn) Type: enhancement Issue is to be solved by implementing new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants