HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Parsing Valve Map Files (VMF) into a tree like structure

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mapintolikevalvefilesparsingstructurevmftree

Problem

I've written a small library of objects for parsing of Valve Map Files (VMF). These files are always generated and exported from either the Hammer World Editor, or a Source engine built in map making program like the PTI in Portal 2. So I can trust them to always look roughly like the following, except of course that these are just bits of what is usually a much more massive file.

in.vmf

visgroups
{
    visgroup
    {
        "name" "Tree_1"
        "visgroupid" "5"
        "color" "65 45 0"
    }
    visgroup
    {
        "name" "Tree_2"
        "visgroupid" "1"
        "color" "60 35 0"
        visgroup
        {
            "name" "Branch_1"
            "visgroupid" "2"
            "color" "0 192 0"
        }
        visgroup
        {
            "name" "Branch_2"
            "visgroupid" "3"
            "color" "0 255 0"
            visgroup
            {
                "name" "Leaf"
                "visgroupid" "4"
                "color" "255 0 0"
            }
        }
    }
}
viewsettings
{
    "bSnapToGrid" "1"
    "bShowGrid" "1"
    "bShowLogicalGrid" "0"
    "nGridSpacing" "64"
    "bShow3DGrid" "0"
}
side
{
    "id" "6"
    "plane" "(512 -512 -512) (-512 -512 -512) (-512 -512 512)"
    "material" "BRICK/BRICKFLOOR001A"
    "uaxis" "[1 0 0 0] 0.25"
    "vaxis" "[0 0 -1 0] 0.25"
    "rotation" "0"
    "lightmapscale" "16"
    "smoothing_groups" "0"
    dispinfo
    {
    }
}


I'm looking for feedback on anything relating to structure of my tree, or how I programmed it. If you have any tips, or see some place where I've taken a longer route than necessary, I'd like to know.

I'm not looking for criticism on how my if-else chains look, or how I only include brackets when necessary instead of all the time to maintain consistency. I am more interested in functionality feedback.

One thing I can point out right now, which I believe is inefficient is that when a block is being created, it is looped over just to be collected and passed into anothe

Solution

public VProperty(string name, string value = "")
public VProperty(string text)


This means that the default value will never be used. If you write new VProperty("propertyName"), it will call the text overload.

To avoid the confusion, consider using some way of differentiate parsing from normal constructor, like using a static factory method (e.g. public static VProperty Parse(string text)), or even have the parsing and possibly also ToVMFString() in a separate class.

'\"'


You don't need the backslash there, '"' works fine.

public VBlock(string name, IList body = null)
{
    Name = name;
    if (body == null)
        body = new List();
}


This constructor won't work, you never actually assign to Body.

You can make members of internal classes public, instead of internal. It will work exactly the same, but it will make it much simpler to make the class public, if you ever decide to do that.

IList nBody = new List();
int depth = 0;
var wasDeep = false;


The name nBody is not very good. What does the n mean? You shouldn't use abbreviations like that to make your code clearer.

Also, there is no need to specify the type for nBody, you could use var here. On the other hand, I would be explicit with wasDeep, I think saving that one character is not worth it.

Code Snippets

public VProperty(string name, string value = "")
public VProperty(string text)
public VBlock(string name, IList<IVNode> body = null)
{
    Name = name;
    if (body == null)
        body = new List<IVNode>();
}
IList<IVNode> nBody = new List<IVNode>();
int depth = 0;
var wasDeep = false;

Context

StackExchange Code Review Q#44895, answer score: 8

Revisions (0)

No revisions yet.