Jump to content
Unity Insider Forum

Codebeurteilung: Mouse Orbit und Zoom


Reggie()

Recommended Posts

Hi. ich habe diesen Code nach viel Rumprobieren gebastelt und er macht endlich, was er soll. Aber der code sieht so .... noobig ... aus. 

Frage: Ist das so richtig oder macht man das normalerweise anders?

public class KameraSteuerung : MonoBehaviour {

    public Transform drehobjekt;
    public Camera maincam;
    public int fieldOfView = 50;
    private float mousex;
    private int minFieldofView = 24, maxFieldofView = 60;

    void Update()
    {

        if (Input.GetMouseButton(1))
        {
            mousex = Input.GetAxis("Mouse X");
            if (mousex > 0)
            {
                drehobjekt.Rotate(new Vector3(0, mousex++, 0));
            } else
            {
                drehobjekt.Rotate(new Vector3(0, mousex--, 0));
            }
        }

        if (Input.GetAxis("Mouse ScrollWheel") > 0 && maincam.fieldOfView > minFieldofView)
        {
            maincam.fieldOfView--;
        }
        if (Input.GetAxis("Mouse ScrollWheel") < 0 && maincam.fieldOfView < maxFieldofView)
        {
            maincam.fieldOfView++;
        }
    }
}

Hier wird weder die Kamera gedreht noch bewegt. man kann die Kamera auf der X-Achse um ein unsichtbares Objekt in der Mitte kreisen und mit dem Mausrad bis zu min und max Werten rein- und rauszoomen.

Link zu diesem Kommentar
Auf anderen Seiten teilen

Da der Code funktioniert, knalle ich einfach mal entsprechend der Anfrage die volle Ladung Kritik raus. Positives lasse ich dabei einfach mal weg - heißt nicht, dass der Code schlecht ist ;)

Die Reihenfolge der Punkte ist beliebig.

  1. Öffnenende geschweifte Klammer
    C#-Standard ist eine öffnende geschweifte Klammer in der nächsten Zeile, wenn es nicht um Einzeiler geht. das machst du schon überall richtig, außer bei der allerersten bei der Klassendefinition. Wenn dein Team aus irgendeinem Grund vom C#-Standard abweichen will, dann ist das erstmal kein Verbrechen - Hauptsache ist aber, konsistent zu sein und es nicht mal so, mal so zu machen.
  2. Sprache
    Du mischt Deutsch und Englisch. Ich würd ganz klar empfehlen, immer alles auf Englisch zu schreiben, aber auch hier ist das Allerwichtigste Konsistenz.
  3. Variablennamen
    Der Standard bei C# und das, was in Unity passiert, unterscheiden sich leider. Öffentliche Felder wie "drehobjekt" werden in C# eigentlich groß geschrieben. Aus Gründen™ ist das in Unity allerdings nicht der Fall. Ich habe mich dafür entschieden, beim Unity-Standard zu bleiben, weil sonst mein Code beim Verwenden der Felder mal so, mal so aussieht. Da krieg ich die Krise.
    In beiden Fällen ist Camel Case angesagt. Wo ein Leerzeichen oder ein Bindestrich hinkäme, gibt's nen Großbuchstaben. Statt "drehobjekt" also "drehObjekt", statt "maincam" also "mainCam", usw. "fieldOfView" ist richtig, "maxFieldofView" nicht.
  4. Öffentliche Felder
    Das Wort "public" macht, dass man Dinge im Unity-Editor einstellen kann. Es ist aber kein Unity-Begriff, sondern hat auch ganz außerhalb von Unity eine Bedeutung. Diese Bedeutung ist, dass andere Klassen Zugriff auf die Felder und Methoden dieser Klasse haben, wenn sie public sind. Das ist oft nicht wünschenswert.
    Ich nehme z.B. stark an, dass dein Camera-Feld öffentlich ist, damit du da etwas hineinzihen kannst - wenn aber andere Klassen daran herumfummeln könnten, könnte in dieser Klasse etwas kaputt gehen.
    Du kannst einstellen, dass Felder im Editor eingestellt können, ohne sie öffentlich zu machen:
    [SerializeField]
    private Type fieldName;
  5. Mehrere Felder in einer Zeile deklarieren
    Bitte nicht :)

  6. else in eigene Zeile

  7. Increment/Decrement innerhalb eines größeren Statements
    Sachen wie "Dinge(zeug++)" verringern die Lesbarkeit deines Codes. Pack das "++" in die nächste Zeile, als eigenständige Anweisung.
    Mal davon abgesehen, dass dein "mousex++" genau gar nichts macht ;) Kannste rausstreichen. Und damit entfällt das if-else auch komplett.

  8. Mathematik statt Kontrollstrukturen
    Du hast mehrere if-Abfragen, die Fälle überprüfen, die am Ende dann Dinge machen, die wiederum in Abhängigkeit ihres Inputs sind.
    Beispiel:

    var input = Input.GetAxis("Horizontal");
    if (input > 0)
    {
      Move(input);
    }
    else
    {
      Move(-input);
    }

    Stattdessen:

    var input = Input.GetAxis("Horizontal");
    Move(Mathf.Abs(input));

    Da das erste if-else schon wegfällt, kannst du mal über das zweite schauen. Dort kannst du mit deinem GetAxis und Mathf.Clamp die beiden ifs einfach streichen.

  9. Komponenten da, wo sie hingehören
    Ich kann aus deinem Script nicht erkennen, auf was für ein GameObject du es legst. Theoretisch könntest du das Script auf ein neues, leeres GameObject packen, die Eigenschaften einstellen und es läuft. Das ist nicht besonders gut, weil so eine Einstellung das auf lange Sicht dazu führt, dass irgendwelche Objekte irgendetwas machen. Deine Szenen werden dann zunehmend konplexer und konfuser, und Dinge passieren und du weißt nicht, welches GameObject dafür verantwortlich ist.
    Ich empfehle, schon beim Schreiben der Scripte zu definieren, wie die Komponente einzusetzen ist. Da es sich hier um ein Script handelt, das eine Kamera beeinflusst, sollte das Script doch auf der Kamera liegen, oder?
    Anstatt also die Kamera von woanders reinzuziehen, benutzen wir die Kamera auf demselben GameObject.

    new private Camera camera; // new, um einer Warnung von Unity vorzubeugen
    
    private void Awake()
    {
      camera = GetComponent<Camera>();
    }

    Das erlaubt uns, im Script zu definieren, dass es ohne Kamera gar nicht erst funktioniert.

    [RequireComponent(typeof(Camera))]
    public class CameraZoom : MonoBehaviour
    {

    Jetzt sagst du "Aber mein Script arbeitet sowohl mit der Kamera, als auch mit einem Parent der Kamera, der sich drehen soll. Es geht hier also um zwei GameObjects". Und damit wären wir bei...

  10. Eine Komponente für eine Aufgabe
    Du hast Kamera-Zoom. Du hast Kameradrehung. Mach dafür 2 Scripts. Wenn eine Klasse eine Aufgabe übernimmt, dann ist das Fehlerpotential geringer und du erhälsts Modularität. Du kannst dann z.B. eine Sondervariante der Kamera bauen, die nicht zoomen kann. Oder eine, die sich nicht drehen kann. Wenn deine Funktionalität nicht zusammengefasst wird, kannst du sie einzeln an- und ausschalten. Das ist die Stärke von Unitys Komponentensystem, aber das ist auch etwas, das du bei OOP immer machen willst.
    Dann gibt es nur noch eines, das mir auffällt.

  11. Räume den Code auf
    Dein Feld "fieldOfView" benutzt du im Code gar nicht ;)

Link zu diesem Kommentar
Auf anderen Seiten teilen

ich habe so lange dran herumgedoktert, bis es endlich genau so funktionierte, wie ich es wollte. eigentlich wollte ich nur wissen, ob diese Art der Lösung gut bzw. effektiv ist, oder ob man das anders/besser/eleganter/kürzer lösen kann und nicht, ob ich "vorschriftsmäßig" gecoded habe. aber ist ok, mein Titel war wohl daran Schuld. und wie gesagt, einiges war wirklich hilfreich.

var input = Input.GetAxis("Horizontal");
Move(Mathf.Abs(input));

das verstehe ich nicht, selbst nach lesen der Dokumentation. Mathf.Abs entfernt doch nur das Vorzeichen. Aber das ist doch notwendig, um die Schwenkrichtung zu bestimmen (minus -> nach links schwenken, sonst nach rechts)

Link zu diesem Kommentar
Auf anderen Seiten teilen

Das war ein Beispiel, das sich auf den Code darüber bezieht. Ob das jetzt sinnvoller Code ist oder nicht, spielt keine Rolle - kann ja ein Spiel geben, das so funktioniert. Der Code, den du zitierst, macht einfach dasselbe wie der Code darüber in meinem Post - nur eben mathmatisch statt über Kontrollstrukturen.

Link zu diesem Kommentar
Auf anderen Seiten teilen

Am 29.1.2019 um 22:58 schrieb Sascha:

Der Standard bei C# und das, was in Unity passiert, unterscheiden sich leider. Öffentliche Felder wie "drehobjekt" werden in C# eigentlich groß geschrieben. Aus Gründen™ ist das in Unity allerdings nicht der Fall. Ich habe mich dafür entschieden, beim Unity-Standard zu bleiben, weil sonst mein Code beim Verwenden der Felder mal so, mal so aussieht. Da krieg ich die Krise.

Unity will  sich jetzt ja auch an die C# Standard halten, aber um nicht alles zu mischen, haben sie sich entschlossen im "alten" Namespace (UnityEngine/UnityEditor) den alten Stil zu verwenden, im neuerem "Unity" Namespace sich an den Standard zu halten.

Link zu diesem Kommentar
Auf anderen Seiten teilen

Archiviert

Dieses Thema ist jetzt archiviert und für weitere Antworten gesperrt.

×
×
  • Neu erstellen...