5
\$\begingroup\$

I am writing my own 3D engine. I initially made the code quick and dirty but after a while the code became pretty messy and it have a lot of repeating code so I am currently cleaning up some of the code that I have written previously, one thing I didn't do originally was creating a base class for my 3D objects and I did not use inheritance for the different 3D object types even though they share a lot of properties and methods, but now when I am cleaning up the code and want to do it as "proper/best possible", I have created a cleaned up version of the class objects by using a base object class and the inheriting from it, but I want to know if I can improve upon it further.

Screenshot of my 3D engine:

I am not using any third party library, drawing viewport with System.Drawing; and Bitmap.SetPixel, rendering is multi-threaded.

I feel like this code could be even cleaner and inherit more from the base class.

  1. For example I have a static "Add" method in the derived classes that are basically the same for all object types with just some additional parameters that differ, but I am not sure how I would inherit a static method, or perhaps there is a smarter way to do it that creates a similar "user programming experience"? In terms of how to use it, i.e Box.Add(), Quad.Add()

  2. For the derived classes could I write the constructor in some cleaner way? Because currently it's a very long line of arguments, and most are the arguments from the base class.

BaseClass:

public abstract class BaseObject
{
    protected BaseObject(Mat4 _tm, Vec3 _pos, Vec3 _rot, double _scl, Color _col, string _name, string _type)
    {
        tm = _tm;
        pos = _pos;
        rot = _rot;
        dir = tm.GetForwardVec();
        scl = _scl;
        col = _col;
        name = _name;
        type = _type;
    }

    public Mat4 tm { get; }
    public Vec3 pos { get; }
    public Vec3 rot { get; }
    public Vec3 dir { get; }
    public double scl { get; }
    public Color col { get; }
    public string name { get; set; }
    public string type { get;}

    private Vec3[] verticesLocal;

    protected abstract Vec3[] SetVertices();

    private Vec3[] p_verticesGlobal;
    public Vec3[] verticesGlobal
    {
        get { return p_verticesGlobal; }
        protected set { p_verticesGlobal = value; }
    }

    private void UpdateVerticesGlobal()
    {
        for (int i = 0; i < verticesLocal.Length; i++)
        {
            verticesGlobal[i] = tm * verticesLocal[i];
        }
    }

    private Line3[] p_edges;
    public Line3[] edges
    {
        get { return p_edges; }
        protected set { p_edges = value; }
    }

    protected abstract void UpdateEdges();

    protected void Initialize()
    {
        verticesLocal = SetVertices();
        verticesGlobal = verticesLocal;
        UpdateVerticesGlobal();
        UpdateEdges();
    }
}

Example Of a couple "3D Object Types" classes:

(I have a lot of different ones, spheres, cylinders, mesh, points, etc..)

Box:

public class Box2 : BaseObject
{
    private Box2(Mat4 _tm, Vec3 _pos, Vec3 _rot, double _scl, Color _col, string _name, string _type, double _width, double _height, double _depth) : base(_tm, _pos, _rot, _scl, _col, _name, _type)
    {
        width = _width;
        height = _height;
        depth = _depth;
        edges = new Line3[12];
        base.Initialize();
    }

    public double width { get; }
    public double height { get; }
    public double depth { get; }

    protected override Vec3[] SetVertices()
    {
        Vec3[] v = new Vec3[8]
        {
            new Vec3(-1, -1, -1),
            new Vec3(1, -1, -1),
            new Vec3(-1, 1, -1),
            new Vec3(1, 1, -1),
            new Vec3(-1, -1, 1),
            new Vec3(1, -1, 1),
            new Vec3(-1, 1, 1),
            new Vec3(1, 1, 1)
        };
        for (int i = 0; i < 8; i++)
        {
            v[i] = new Vec3(v[i].x * width * 0.5, v[i].y * height * 0.5, v[i].z * depth * 0.5);
        }
        return v;
    }

    protected override void UpdateEdges()
    {
        Vec3[] v = verticesGlobal;
        edges[0] = new Line3(v[0], v[1], col);
        edges[1] = new Line3(v[2], v[3], col);
        edges[2] = new Line3(v[4], v[5], col);
        edges[3] = new Line3(v[6], v[7], col);
        edges[4] = new Line3(v[0], v[2], col);
        edges[5] = new Line3(v[1], v[3], col);
        edges[6] = new Line3(v[4], v[6], col);
        edges[7] = new Line3(v[5], v[7], col);
        edges[8] = new Line3(v[0], v[4], col);
        edges[9] = new Line3(v[1], v[5], col);
        edges[10] = new Line3(v[2], v[6], col);
        edges[11] = new Line3(v[3], v[7], col);
    }

    public static Box2 Add(Vec3? _pos = null, Vec3? _rot = null, double _scl = 1, Color? _col = null, string _name = "Box_", bool _add = true, double _width = 100, double _height = 100, double _depth = 100)
    {
        Vec3 p = _pos ?? new Vec3(0, 0, 0);
        Vec3 r = _rot ?? new Vec3(0, 0, 0);
        Color c = _col ?? Color.FromArgb(0, 0, 0);
        Mat4 tm = Mat4.PosRotScaleTM(p, r, new Vec3(_scl, _scl, _scl));
        Box2 o = new Box2(tm, p, r, _scl, c, _name, "Box", _width, _height, _depth);
        if (_add)
        {
            RenderEngine.scene.baseObjects.Add(o);
        }
        return o;
    }

Quad:

public class Quad : BaseObject
{
    private Quad(Mat4 _tm, Vec3 _pos, Vec3 _rot, double _scl, Color _col, string _name, string _type, double _width, double _height) : base(_tm, _pos, _rot, _scl, _col, _name, _type)
    {
        width = _width;
        height = _height;
        edges = new Line3[4];
        base.Initialize();
    }

    public double width{ get; }
    public double height { get; }

    protected override Vec3[] SetVertices()
    {        
        Vec3[] v = new Vec3[4]
        {
            new Vec3(-1, -1, 0),
            new Vec3(1, -1, 0),
            new Vec3(-1, 1, 0),
            new Vec3(1, 1, 0)
        };
        for (int i = 0; i < 4; i++)
        {
            v[i] = new Vec3(v[i].x * width * 0.5, v[i].y * height * 0.5, 0);
        }
        return v;
    }

    protected override void UpdateEdges()
    {
        Vec3[] v = verticesGlobal;
        edges[0] = new Line3(v[0], v[1], col);
        edges[1] = new Line3(v[2], v[3], col);
        edges[2] = new Line3(v[0], v[2], col);
        edges[3] = new Line3(v[1], v[3], col);
    }

    public static Quad Add(Vec3? _pos = null, Vec3? _rot = null, double _scl = 1, Color? _col = null, string _name = "Quad", bool _add = true, double _width = 100, double _height = 100)
    {
        Vec3 p = _pos ?? new Vec3(0, 0, 0);
        Vec3 r = _rot ?? new Vec3(0, 0, 0);
        Color c = _col ?? Color.FromArgb(0, 0, 0);
        Mat4 tm = Mat4.PosRotScaleTM(p, r, new Vec3(_scl, _scl, _scl));
        Quad o = new Quad(tm, p, r, _scl, c, _name, "Quad", _width, _height);
        if (_add)
        {
            RenderEngine.scene.baseObjects.Add(o);
        }
        return o;
    }
}

Program:

Box.Add(); // Creates object with default parameters 
Quad.Add(_pos: new Vec3 (0,200,50), _col:Color.Yellow) // Override selected default parameters 
\$\endgroup\$
  • \$\begingroup\$ Why is your coding style the opposite of the usual C# one? (I'm taking about using underscores for parameters.) \$\endgroup\$ – BCdotWEB May 7 at 9:02
  • \$\begingroup\$ @BCdotWEB I do it to separate local variables from parameters, easier for me to read , also it helps a bit with code completion, e.g typing Box(_) shows all available parameters. \$\endgroup\$ – Patrik Fröhler May 8 at 0:05
  • 1
    \$\begingroup\$ But your style is the opposite of the conventional C# style, where an underscore indicates a private variable and parameters do not have this. \$\endgroup\$ – BCdotWEB May 8 at 9:51
  • \$\begingroup\$ True, I should probably change to be more conventional. \$\endgroup\$ – Patrik Fröhler May 8 at 17:53
3
\$\begingroup\$

I am not sure how I would inherit a static method

As I'm sure you've figured out, you can't. I also don't think this would make any sense in your case, since the APIs is not the same. If you are just wanting to reduce the amount of code that these Add methods have in common, then hopefully some of my suggestions below will help.

tm Redundancy

Why do you need pos, rot, and scl along with tm? As best I can tell, these are redundant, and redundancy requires effort to maintain. However, I'm guessing you have a good reason (e.g. tm is useful for computing vertices for actual rendering, while the others are useful for querying and diagnostic rendering). I would be inclined to think very hard about whether tm will always contain the same information as the others, and if every class is going to include Mat4 tm = Mat4.PosRotScaleTM(p, r, new Vec3(_scl, _scl, _scl)); I would consider packaging all this information up in a struct, maybe called Orientation or something (only suggesting a name because I use it below).

This will reduce the number of little parameters being passed around, reduce code-duplication a little (maybe not-insignificantly if you have other classes which use the same sort of information), and replace those long lines which are full of confusion with something that says "I'm just packaging these up so that I can supply them to the base-class" without any risk of getting it wrong. There is a risk that this will just create unnecessary structures, but I think it's worth considering. It would be most useful if you could change the orientation of objects (because you could swap-out everything at once, or provide (potentially efficient) mutator methods for common operations which maintain consistency), but it looks like that isn't a concern here.

Correctness

This line is suspect:

verticesGlobal = verticesLocal;

I'm guessing you really meant something like verticesGlobal = verticesLocal.ToArray() (LINQ style copy). Currently have the same array in 2 variables and change the contents across 2 methods and it's all very confusing and misleading and I'm sure it's not what you intended.

Initialisation

It's weird that SetVerticies doesn't set anything, generally that it has a different API from UpdateEdges, when they perform the same kind of role.

Should verticesGlobal._set be private rather than protected?

Separation of concerns

Note that none of your derived classes (those that I can see) ever look at anything they send down to BaseObject's constructor: there isn't really a dependency here (only Edges is shared). Generally, the whole thing feels like you are packing too much logic into BaseObject: would it make sense to separate 'objects' from their 'geometry'? That way, you could construct some Quad geometry, which doesn't care about position or scaling, and simply provides methods to obtain some verticies and some edges (maybe defines edges as indexes into the vertex array so you don't need any fancy initialisation?). Then your object class can just combine some position information (tm and such) with some geometry info, all of which could be performed in the constructor, because the 'geometry' object is initialised before the 'object' object.

There are many ways you could facilitate such a separation; I'd be inclined to create one well-defined layer of abstraction for geometry, so you have the freedom to hide whatever you want behind it.

public interface IGeometry
{
    /// <summary> Generates model-space vertices for the geometry </summary>
    Vec3[] GetVertices();

    /// <summary> Generates edges for the geometry from world-space coordinates </summary>
    Line3[] GetEdges(Vec3[] vertices, Color col);
}

The object class constructor might then look more like:

public Object(Orientation _orientation, IGeometry _geometry string _name, string _type)
{
    orientation = _orientation;
    geometry = _geometry;
    col = _col;
    name = _name;
    type = _type;

    verticesLocal = geometry.GetVertices();
    verticesGlobal = transform(orientation, verticesLocal);
    edges = geometry.GetEdges(verticesGlobal, col);
}

... in fact that would be the entire class excepting the transform method (which could be provided by Orientation, or indeed the Mat4): it's just drawing together the different concerns for a particular object.

I'm not convinced col really fits in here... but I'd expect material information to be separated from the geometry and position information somehow, but without changing how Edges are consumed that will be difficult.

If you still want to be able to pass around a Box object and interrogate its Boxness, then you can make the 'object' class generic on the type of geometry. The downside is that to pass around an 'object' whose geometric type you don't care about you need an interface, either a geometry independent one, or a covariant one (but that isn't necessarily a bad thing).

I should stress that I've not thought about this very hard, and this change may not fit into your complete project, or indeed it may not go far enough in separating the concerns: it's hard to tell without more context.

Naming

What is tm? Transformation matrix? Is it world space or camera space or model space or what?

What is scl? Scale?

Why is it a Box2? I can't see anything 2 about it.

Typical .NET naming conventions make all public members ProperCamelCase, e.g. VerticesGlobal, Scl, Vec3.X. Underscores are also usually reserved for private fields and throw-away variables.

Documentation

It would be nice to see some more inline documentation (///) on some of these methods, in particular the ones which are to be inherited. For example, how can someone implementing UpatedEdges be sure that verticesGlobal is in a useful state? It's not written anywhere, so they shouldn't rely upon it, but it's clearly your intention to provide this option. If you want to keep short and familiar variable names like rot and scl, then documentation would also provide a way to expand upon exactly what these things are, so that people less familiar with the system can work out how to use it more easily.

\$\endgroup\$
  • \$\begingroup\$ #0 storing pos, rot,scl, +tm is mostly for user interaction but, also is an issue extraction euler rotation angles from a transformation matrix. #1 yes should beverticesGlobal = verticesLocal.ToArray() was a miss by me, would for sure have became an issue later . #2 Box2 was just a temp name so I would not mess up the rest of my code that is dependent on the original box class, I was meant to change that after I pasted the code here. I posted a new version of my code as an answer, but I was not sure how i would use the interface, would be nice if you elaborated a bit on it \$\endgroup\$ – Patrik Fröhler May 7 at 23:57
1
\$\begingroup\$

Here is an attempt at an updated version based on @visualmelon suggestions, but not sure If I just ended up making bigger mess of it then before or not, partly because I'm not very familiar with interfaces and not sure how to properly use it to make this simpler (I would happily take some more suggestions), though this version is a bit more fleshed out then last version with the ability to modify and move the object after it is constructed, so is going to be a bit more code for the extra functionality. (though the extra functionally is not really working yet because I'm not sure how I am going to update the vertices )

This code is a mess, going to have to re-think it a bit more, but I'll post this for now.

Changes:

  • I created a Transform class containing position, rotation, scale, and transform matrix.

  • I created a Edge struct and define them as vertex index.

  • I created a Geometry class containing worldSpaceVertices, localSpaceVertices, and edges. (storing both world, and local space vertices for performance reasons)

  • Change my viewpoint draw function to be able to draw the edges

  • Change my naming convention a bit and use more descriptive naming instead of shorthands

BaseObject:

public abstract class BaseObject
{
    protected BaseObject(Transform _transform, Geometry _geometry, string _type, Color _wirecolor, string _name)
    {
        transform = _transform;
        geometry = _geometry;
        wirecolor = _wirecolor;
        name = _name;
        type = _type;
    }

    public Transform transform { get; }
    public Geometry geometry { get; }
    public Color wirecolor { get; set; }
    public string name { get; set; }
    public string type { get; }
}

Transform Class:

public class Transform
{
    public Transform(Vec3 _positon, Vec3 _rotation, double _scale)
    {
        position = _positon;
        rotation = _rotation;
        scale = _scale;
        UpdateTransformMatrix();
    }

    public Transform(Mat4 _tm)
    {
        matrix = _tm;
        UpdateTransform(_tm);
    }

    public Vec3 position { get; private set; }
    public Vec3 rotation { get; private set; }
    public double scale { get; private set; }
    public Mat4 matrix { get; private set; }

    public void SetTransform(Vec3? _position = null, Vec3? _rotation = null, double? _scale = null)
    {
        position = _position ?? position;
        rotation = _rotation ?? rotation;
        scale = _scale ?? scale;
        UpdateTransformMatrix();
    }

    public void SetTransform(Mat4 _tm)
    {
        matrix = _tm;
        UpdateTransform(_tm);
    }

    private void UpdateTransformMatrix()
    {
        matrix = Mat4.PosRotScaleTM(position, rotation, scale);
        // need to add a way to update vertecies
    }

    private void UpdateTransform(Mat4 _tm)
    {
        position = Mat4.GetPos(matrix);
        rotation = Mat4.GetRot(matrix);
        scale = Mat4.GetScl(matrix);
        // need to add a way to update vertecies
    }

    public Vec3 GetDir()
    {
        return (matrix.GetForwardVec());
    }
}

Edge Struct:

public struct Edge
{
    /// <summary> Edge start and end, worldSpaceVertex index. </summary>
    public Edge(int _startVertexIDX, int _endVertexIDX)
    {
        startVertexIDX = _startVertexIDX;
        endVertexIDX = _endVertexIDX;
    }

    public int startVertexIDX { get; }
    public int endVertexIDX { get; }
}

Geometry Class:

public class Geometry
{
    public Geometry(Vec3[] _verticesLocal, Edge[] _edges, Mat4 _tm)
    {
        verticesLocal = _verticesLocal;
        verticesGlobal = _verticesLocal.ToArray();
        edges = _edges;
        UpdateGlobalVertices(_tm);
    }

    private Vec3[] p_verticesLocal;
    public Vec3[] verticesLocal
    {
        get { return p_verticesLocal; }
        private set { p_verticesLocal = value; }
    }

    public void SetLocalVertices(Mat4 _tm, Vec3[] _verticesLocal)
    {
        p_verticesLocal = _verticesLocal;
        UpdateGlobalVertices(_tm);
    }

    private Vec3[] p_verticesGlobal;
    public Vec3[] verticesGlobal
    {
        get { return p_verticesGlobal; }
        private set { p_verticesGlobal = value; }
    }

    public void UpdateGlobalVertices(Mat4 _tm)
    {
        for (int i = 0; i < verticesLocal.Length; i++)
        {
            verticesGlobal[i] = _tm * verticesLocal[i];
        }
    }

    private Edge[] p_edges;
    public Edge[] edges
    {
        get { return p_edges; }
        private set { p_edges = value; }
    }
} 

Quad:

   public class Quad : BaseObject
    {
        private Quad(Transform _transform, Geometry _geometry, string _type, Color _wirecolor, string _name, double _width, double _height) : base(_transform, _geometry, _type, _wirecolor, _name)
        {
            width = _width;
            height = _height;
            UpdateVertices();
        }

        private double p_width;
        public double width
        {
            get {return p_width; }
            set
            {
                p_width = value;
                UpdateVertices();
            }
        }

        private double p_height;
        public double height
        {
            get { return p_height; }
            set
            {
                p_height = value;
                UpdateVertices();
            }
        }

        public static Vec3[] ShapeVertices { get; } = new Vec3[4]
        {
            new Vec3(-1, -1, 0),
            new Vec3(1, -1, 0),
            new Vec3(-1, 1, 0),
            new Vec3(1, 1, 0)
        };

        public static Edge[] ShapeEdges { get; } = new Edge[4]
        {
            new Edge(0, 1),
            new Edge(2, 3),
            new Edge(0, 2),
            new Edge(1, 3)
        };

        private void UpdateVertices()
        {
            Vec3[] v = ShapeVertices.ToArray();
            for (int i = 0; i < 4; i++)
            {
                v[i] = new Vec3(v[i].x * width * 0.5, v[i].y * height * 0.5, 0);
            }
            geometry.SetLocalVertices(transform.matrix, v);
        }

        public static Quad Add(Vec3? _position = null, Vec3? _rotation = null, double _scale = 1, Color? _wirecolor = null, string _name = "Quad", bool _add = true, double _width = 100, double _height = 100)
        {
            Vec3 p = _position ?? new Vec3(0, 0, 0);
            Vec3 r = _rotation ?? new Vec3(0, 0, 0);
            Color c = _wirecolor ?? Color.Black;
            Transform tm = new Transform(p, r, _scale);
            Geometry geo = new Geometry(ShapeVertices, ShapeEdges, tm.matrix);
            Quad o = new Quad(tm, geo, "Quad", c, _name, _width, _height);
            if (_add)
            {
                RenderEngine.scene.baseObjects.Add(o);
            }
            return o;
        }

    }

Box:

public class Box : BaseObject
{
    private Box(Transform _transform, Geometry _geometry, string _type, Color _wirecolor, string _name, double _width, double _height, double _depth) : base(_transform, _geometry, _type, _wirecolor, _name)
    {
        width = _width;
        height = _height;
        depth = _depth;

        UpdateVertices();
    }

    private double p_width;
    public double width
    {
        get { return p_width; }
        set
        {
            p_width = value;
            UpdateVertices();
        }
    }

    private double p_height;
    public double height
    {
        get { return p_height; }
        set
        {
            p_height = value;
            UpdateVertices();
        }
    }

    private double p_depth;
    public double depth
    {
        get { return p_depth; }
        set
        {
            p_depth = value;
            UpdateVertices();
        }
    }

    public static Vec3[] ShapeVertices { get; } = new Vec3[8]
    {
        new Vec3(-1, -1, -1),
        new Vec3(1, -1, -1),
        new Vec3(-1, 1, -1),
        new Vec3(1, 1, -1),
        new Vec3(-1, -1, 1),
        new Vec3(1, -1, 1),
        new Vec3(-1, 1, 1),
        new Vec3(1, 1, 1)
    };

    public static Edge[] ShapeEdges { get; } = new Edge[12]
    {
        new Edge(0, 1),
        new Edge(2, 3),
        new Edge(4, 5),
        new Edge(6, 7),
        new Edge(0, 2),
        new Edge(1, 3),
        new Edge(4, 6),
        new Edge(5, 7),
        new Edge(0, 4),
        new Edge(1, 5),
        new Edge(2, 6),
        new Edge(3, 7)
    };

    private void UpdateVertices()
    {
        Vec3[] v = ShapeVertices.ToArray();
        for (int i = 0; i < 8; i++)
        {
            v[i] = new Vec3(v[i].x * width * 0.5, v[i].y * height * 0.5, v[i].z * depth * 0.5);
        }
        geometry.SetLocalVertices(transform.matrix, v);
    }

    public static Box Add(Vec3? _position = null, Vec3? _rotation = null, double _scale = 1, Color? _wirecolor = null, string _name = "Box", bool _add = true, double _width = 100, double _height = 100, double _depth = 100)
    {
        Vec3 p = _position ?? new Vec3(0, 0, 0);
        Vec3 r = _rotation ?? new Vec3(0, 0, 0);
        Color c = _wirecolor ?? Color.Black;
        Transform tm = new Transform(p, r, _scale);
        Geometry geo = new Geometry(ShapeVertices, ShapeEdges, tm.matrix);
        Box o = new Box(tm, geo, "Box", c, _name, _width, _height, _depth);
        if (_add)
        {
            RenderEngine.scene.baseObjects.Add(o);
        }
        return o;
    }
}

Draw:

    public static class Draw
    {
        public static void Line(Vec3 _p0, Vec3 _p1, Pen _pen)
        {
            Camera.WorldLineToScreen(_p0, _p1, ref xySS0, ref xySS1); 
            RenderEngine.g.DrawLine(_pen, (float)xySS0.x, (float)xySS0.y, (float)xySS1.x, (float)xySS1.y);
        }

        private static Pen basePen = new Pen(Color.Black);
        public static void BaseObject(BaseObject _obj)
        {
            basePen.Color = _obj.wirecolor;
            foreach (Edge e in _obj.geometry.edges)
            {
                Draw.Line(_obj.geometry.verticesGlobal[e.startVertexIDX], _obj.geometry.verticesGlobal[e.endVertexIDX], basePen);
            }
        }
}

Program:

 Box.Add();
 Quad.Add(_wirecolor:Color.Yellow, _position:new Vec3(200,50,0));
\$\endgroup\$
  • \$\begingroup\$ If you would like more detailed feedback on your new code, then you should probably post it as a new question; though you'd have to get it working first. The Q&A format doesn't really support a running conversation: see this help-centre article for more info. All that said, the Geometry class wasn't really what I had in mind (though I rather like it), and it's more complicated if you to be able to change the geometry without manually invoking an update. \$\endgroup\$ – VisualMelon May 8 at 9:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.