Usability limitations, code readability and others

[peteihis]

Some issues left to deal with

Coding style:

• The code is rather unreadable, blocks are not visually separated by indentation levels, lines are divided into several lines unnecessarily (hey we have widescreen displays nowadays) etc...
• Mixed use of spaces and tabs
• Some level of renaming would help understanding

Code itself:

• Uses occasional outdated parts of the core software (ModellingApp, are there others?)
• Writes (not many but it does) uninformative comments to System.out, when it is just doing what it is supposed to. Originally debugging code I guess?
• Should set smoothing of the imported meshes to "none". The default setting shades flat surfaces as curved, which looks weird.

Usability:

• At import centering works properly in one special case only. From negative coordinates centering does not happen at all.
• At export objects are translated to all-positive coordinates. This follows the idea on original STL spec, but as the function alters the geometry data, it should be the user's choice.
• Filename "Untitled" is written in the code and pops up every time though other UI setting are remembered during session.
• File type-extension filters for just .stl would be welcome
• The dialog elements should be lined-up, not drifting left and right
• Plugin specific translations are missing. Not everything can be found in AoI dictionary.

There's probably more but already the above could take couple of PRs to deal with. Also different functions could be split into different files just to simplify maintenance, but let's get it right first.

One long term idea could be to add a graphic pre- and post processor to set scaling, orientation, triangulation etc... so you would know what to expect and could avoid trial an error rounds.

[peteihis]

BTW, any specific place for libraries in gradle-builds? Does it have a fixed name?

EDIT: STLTranslator uses Buoy, but it somehow finds that in the core AoI folder?

[Sailsman63]

Gradle uses the word "dependencies", and has standard ways to either download them from, eg, maven central or to look at local files. The later is what this is doing with aoiLocation.

ArtOfIllusion.jar has all of its core libraries listed in the classpath entries of its manifest, so we only have to explicitly pull in the one jar.

[peteihis]

Question:

Does this structure make any sense?

    thread = new Thread(new Runnable()
    {
        public void run()
        {
            try
            {
               // .... code

                if (thread == null) // ??? Inside the thread itself?
                    return;

                // more code ....
            }
            // catch ...
        }
    }

This is what I found in there.

[Sailsman63]

It does... sort of. The variable thread would get set to null if cancel() was called while the import/export process was running. It allows the try-block to bail out early if the user cancels, and especially skip displaying the error messages.

I think that there are probably cleaner/better ways to do this, but they might not have existed when the code was written.

[peteihis]

I see, I'll keep those then.

There is a pretty stupid (old) bug, that when I open the importer and then cancel it, it creates a new LayoutWindow with an empty scene anyway. There is not much file structure here to get a grip on, everything happens more or less on the go .... At first sight I can not see why that happens.

Also, somehow I'm not sure it it is the best idea to have a plugin create a new LayoutWindow (that it will be a part of) and then respond to the SCENE_WINDOW_CREATED message, hoping that it recognizes the window to be it's own creation. But let's see where this leads to...

EDIT:

At first sight I can not see why that happens.

I think I got it.... Some restructuring needed like, create the scene when you know that you need it and not as the first thing ever just to be sure it's there.... :/ What the rest of the code thinks of that remains to be seen....

[Sailsman63]

I'd seriously consider restructuring/breaking up the code into more sensible units before trying to patch past a bug like that. Once the structure is more clear, what's going wrong should be easier to fix.

[peteihis]

I'd seriously consider restructuring/breaking up the code

Trust me, so would I! :D ... Let's just say that the wiring needs untangling before it is safe to start cutting the circuit board. This empty-scene bug was actually a relatively easy thing to fix once I got to the trail of it. It is mostly the UI code that needs restructuring. The current one is handling both import and export through the same method though they never happen from the same launch.

[peteihis]

Update: 13 commits so far. Export and import are now separated in variables level, I'd expect that to make splitting the code into smaller pieces easier. The next thing (and very possibly close to the last for a while) would be to rework the UI.

[peteihis]

I had a look a the code since a long time. The original code was actually put together very efficiently but with the drawback that it was close to impossible to separate the functions from each others. Another efficient bit is the use of StreamTokenizer, which brought me to the next efficient bit:

readVec(token, vert);
index = (Integer) vmap.get(vert.toString());
if (index != null)
    face[vertno++] = index.intValue();

Vertices of the triangles are read with the immediate assumption that two vertices with matching coordinates are the same vertex. The problem is that they might not be. It is not uncommon that the original export contains several volumes (or a form that folds back on itself) with vertices with the exact same coordinates belonging to different/opposite surfaces. So while the exported model is still completely legal, in those cases it will automatically be defected at import.

This of course is not what the STL-format was developed for, so what can you expect.... But I'd propose a process where all triangles are first read in as they are, then volumes would be created by comparing edges of the triangles. Obviously, if both ends of an edge match but the orientation is NOT opposite, neither those edges nor their end points would be merged. It would still not be 100% fool proof but a lot better at processing those cases.

And as I recall, something similar was happening with the export as well. Generally speaking the approach has been that the data is processed on the go, which always leads to misinterpretations somewhere. What should happen is that at export, the data to export is created first, then checked, errors fixed and only then exported. Similarly the imported data should be first imported entirely, then, without taking shortcuts that might lose information, processed into sets that represent the objects and only then sent to the object constructor.

But all in it's time. If I get forward with this, the next thing to do would be to start to write the functions into separate files. The root level of writing and reading files seems to be usable as it is. The thinking as whole seems to have been from the code efficiency point of view (with the script usage examples and all) but I'd shift the balance to what the "dumb user" (or then a knowlegeable one..) sees.