plattysoft / ArToolKitJpctBaseLib

Other
20 stars 13 forks source link

Marker configuration string #5

Open michaelboyles opened 7 years ago

michaelboyles commented 7 years ago

This is an issue related to ARToolkit but, as this library is intended to make things easy for the user, perhaps we can provide some additional utility classes.

ARToolkit includes three types of marker configuration strings:

single;path_to_pattern_file;pattern_width
multi;path_to_mutli_config_file
nft;path_to_pattern_file;pattern_width

These are very poorly documented on ARToolkit's website and if you have lots of marker types you could end up littering your code with specially formatted strings with no contextual information about what the different parts represent. E.g.

private String marker = "single;data/pattern;80";

'Single' what? '80' what?

I propose we add a utility to encapsulate this special formatting:

public interface MarkerConfigString
{
    String getNative();
}

public class SingleMarkerConfigString implements MarkerConfigString
{
    private static final String FORMAT = "single;%s;%d";

    public SingleMarkerConfigString(final String pathToPatternFile, final int patternWidth)
    { /* set fields */ }

    public String getNative()
    {
        return String.format(FORMAT, pathToPatternFile, patternWidth);
    }
}

// etc.

If we then update TrackableObject3D to take a MarkerConfigString rather than a regular string, it's immediately obvious for a user what the available options are by looking up the concrete implementations. Additionally, they don't have to read the documentation to find out what format the string needs to be in.

plattysoft commented 7 years ago

This is a very good point. I was thinking of a similar approach, but never got it implemented.

My idea was very similar but using enums for single / nft / multi as part of the constructor instead of different classes.

michaelboyles commented 7 years ago

Glad we're on the same page. I've actually already added these classes locally as well as some unit tests so I'll do a pull request at some point.

What are your thoughts on changing the constructor for TrackableObject3D? I'll add a new constructor taking MarkerConfigString as a parameter, obviously, but should we additionally mark the String constructor as deprecated? Depending how this lib is versioned we could scrap the String constructor altogether but I'm unsure whether that's a good idea.

plattysoft commented 7 years ago

I don't think many people is using this library, marking it as deprecated is probably the best idea, it does not feel like a change to trigger a major version name change.

I'd suggest to call the class just MarkerConfig, there is no need to highlight that it will be converted to a string when passed to ARToolkit.

michaelboyles commented 7 years ago

Alright, sounds good.

Yep, that's true.

I'll send a PR soon :)