[DRAFT] Initial work on allowing Model to output to Data

[iKenndac]

This PR relates to issue #11.

This is early work on allowing Model to output to an in-memory representation rather than always to disk.

Since Model's initialiser was very side-effecty (creating an instance of Model created a file on disk), this is a breaking API change. I've added two public functions:

• generateInMemoryFile(…) -> InMemoryFile? returns the new InMemoryFile struct, which contains file data and a path extension.

• writeToFile(…) -> URL? writes to disk as before, and returns the URL to that file.

I'd like feedback on this before I go further, since this breaks all existing usages of Model and needs a ton of documentation updates (wiki, examples, etc). I also think this change makes the isCollectingModels flag obsolete, but I'm not super familiar with the project.

Another thought I had would be to add free functions that mimic the old API, as this project does with the Project type.

Let me know how you feel about the proposed changes, and I'll be happy to adapt and move forward.

[tomasf]

Nice. This makes sense. The main concern is the breaking change. The way the Model initializer currently works is a bit strange. From a technical perspective, doing the heavy lifting in the initializer is not ideal. Developers don't expect initializers to behave like that. But from a more declarative DSL-style perspective, it reads nicely, and that's why I want to keep it.

There are a few backwards-compatible options I can think of:

• Adding a separate Model initializer for this purpose that doesn't call build(), which you could call a method like generateInMemoryFile(...) on. Fairly straightforward if we can figure out a meaningful initializer. But then what would happen if you use the old initializer AND generateInMemoryFile?
• Adding a new build BuildDirective case for a handler that receives the file data and let ModelContentBuilder construct it. We write to disk unless we find an output handler. Something like:


  await Model("foo") {
      Box(10)
      Output { data in
          // do something with data
      }
  }

  The upside is that it fits rather well into the existing style. The downside is, of course, that it makes it more awkward to just get the damn data out.
• Keeping the API entirely separate from Model. Something that takes a name (perhaps optional?), ModelOptions... and a ModelContentBuilder. Internally, it could construct a Model through an internal initializer (that doesn't call build()) with some custom way to handle the output. (Or, extract the code from Model into something common that both Model and this new code uses?) Usage:

  let myData = ModelDataGeneratorThing(name: "foo", options: ...) {
      Box(10)
  }.generateData()

  The more I think of it, the more I'm leaning towards this option. The two APIs live side by side, each doing their own things. One downside is that it limits us to a single model at a time, and we don't get the cache sharing advantage that Project has. If you're generating multiple similar models, sharing a cache can speed things up significantly. But I suppose we could make it take an optional parameter that wraps an EvaluationContext or something like that.

What do you think?

[iKenndac]

I'd personally argue that a struct initialiser that writes a file to disk and then puts the current application in the background by revealing something in the Finder (or Explorer, etc) is extremely weird, and it's OK to get rid of that over time. Having Model and ModelDataGeneratorThing as separate structs would also be a huge amount of duplication - the only difference is whether the data goes to disk or stays in memory - the logic is otherwise identical.

What we could do is keep the existing API and initialiser, but mark it as deprecated. This won't break existing projects, and will nudge users towards using a new initialiser then either generateInMemoryFile() or writeToFile().

So it'd be something like:

await Model("MyCoolModel") { … } // WARNING: Deprecated. Use Model(named:) and .writeToDisk() or .generateData() instead.

let url = await Model(named: "MyCoolModel") { … }
    .writeToDisk()

let fileContents = await Model(named: "MyCoolModel") { … }
    .generateData()

This keeps backward compatibility and moves over to giving the client choice of what happens with very minimal impact on the rest of the project.

What do you think?

[tomasf]

In the vast majority of cases, writing to disk is what people need. It's the current workflow for previewing while developing a model, and it's how you get something to give a slicer to print. I designed this use case to be as lightweight as possible – just a simple call to Model with a name and the geometry in a result builder. Getting the model as data is awesome, but I'm not convinced adding boilerplate to the common case to enable it is the way to go.

[iKenndac]

Ok, how about this:

A new, optional parameter to the initialiser, something like automaticallyWritesToDisk: Bool = true. Existing code (or passing true) keeps the existing behaviour. If false is passed, the additional methods that I added remain so the client can be explicit with what they want to do.

So:

await Model("MyCoolModel") { … } // Same behaviour and API as now

let fileContents: Data = await Model("MyCoolModel", automaticallyWritesToDisk: false) { … }
    .generateData()

let url: URL = await Model("MyCoolModel", automaticallyWritesToDisk: false) { … }
    .writeToDisk()

The keeps the existing behaviour as the default, and adds options for those who want more control. What do you think?

[tomasf]

This looks like a good solution! 👍

[iKenndac]

Alright, I've made some changes. I'm thinking about three use cases:

Question: I saw that the build() method would always return a nil URL if the file already exists, which seems like an odd decision. I left it be, but it's an unusual behaviour for someone using the library and wants to know where the file went. Can I ask why?

1. The existing use case of writing to a directory and having the filename be chosen by the SDK. For this, the default behaviour remains unchanged:

Model("MyCoolModel") { … } // Will write to the current working directory

For folks wanting more control for whatever reason, I added the method .writeToDirectory().

let directoryUrl: URL = …

// NOTE: This will always return `nil` if the file already exists?
let fileUrl: URL? = Model("MyCoolModel", automaticallyWriteToDisk: false) { … }
                        .writeToDirectory(directoryUrl)

2. The use case of someone wanting to use a system save panel or otherwise choose exactly where the output goes - great for adding an "Export Model…" menu item in a GUI app, etc. For this, I added .writeToFile(). This throws in consistency with other file writing APIs around the system.

let fileUrl: URL = … // Present a system save dialog etc
let fileName: String = fileUrl.lastPathComponent.deletingPathExtension

try Model(fileName, automaticallyWriteToDisk: false) { … }
        .writeToFile(fileUrl)

3. The use case of someone wanting to gather the file contents without them going straight to disk. Great for generating models in a Linux server and letting the user download them. For this, I added .generateData().

let file: InMemoryFile? = try Model("MyCoolModel", automaticallyWriteToDisk: false) { … }
                                  .generateData()

let fileName: String = file?.suggestedFileName
let fileContents: Data = file?.contents

Both writeToFile() and writeToDirectory() have the optional parameter revealInSystemFileBrowser, defaulting to false (the logic for the default value being that if someone is taking control of how the model files are being dealt with, they're likely to want to control if the files are revealed in the filesystem browser themselves).

Let me know what you think. If you're happy with the code changes, I'll update all the documentation etc.

[iKenndac]

I also realised I probably should've pointed this PR at dev rather than main, sorry about that. I did pull in the latest changes to main and fix the conflicts, so I think this can be re-pointed at dev easily.

[tomasf]

Question: I saw that the build() method would always return a nil URL if the file already exists, which seems like an odd decision. I left it be, but it's an unusual behaviour for someone using the library and wants to know where the file went. Can I ask why?

The reason is tied to Cadova’s reveal workflow. New models are automatically revealed in the file browser only when they don’t already exist, which makes it easy to immediately open them in Cadova Viewer (or another viewer). On subsequent updates, the expectation is that the viewer reloads the existing file and we don't want to interrupt the user by revealing again.

When generating via a Project, it collects all new model files and reveals them in one go. Because that reveal behavior is driven by whether a model is newly created, build() only returns a URL for new models. Returning nil signals that nothing new was produced.

This is also why I’ve been a bit cautious about reusing Model for other use cases. It has a number of workflow-specific side effects: it logs to stdout, catches and prints errors instead of throwing, and generally assumes a "developer-at-the-terminal" context. All of that is intentional and useful for Cadova itself, but may be less ideal when Cadova is embedded into something else (like an app or a web service).

Your code looks solid. I’m still wondering whether stretching Model to cover both use cases is the right trade-off, versus introducing a separate, more neutral abstraction for the data-oriented use case.

[iKenndac]

I don't really think that Model itself needs to be split up. Ultimately, it generates an array of bytes (file contents) and a file name. Whether that then gets written into a directory, to an explicit file, or into RAM is a very minor difference.

Ignoring preexisting API, I think a sensible design would be:

• Model wouldn't have any side effects on init.
• .writeToFile() and .generateData() remain.
• The CLI case is something like method like .writeToWorkingDirectory(…), and all of the stdout etc stuff happens in there. That allows the others to throw and whatnot, and the CLI handler can behave as a CLI should.
• OR the CLI case could be a very simple wrapper/method that matches the existing API in everything but name: WorkingDirectoryModel("MyCoolModel") { … }.

At any rate, I generally really don't think that one extra line to instruct a model to write to disk etc is enough to be considered boilerplate.

Since you want to keep the existing API, that's a bit more complex since before this PR, the thing called Model isn't a model, it's a CLI main() function. Without renaming it or otherwise changing the API, it's going to be a bit awkward no matter what for folks who want to build something other than a CLI app.

Here's my proposal for a middle-ground:

• New enum:

enum DirectoryWriteResult {
    case newFileWritten(URL)
    case existingFileOverwritten(URL) 
}

• buildToFile() (and therefore the new public func writeToDirectory() returns a DirectoryWriteResult?. This allows the file reveal logic to stay as-is, but clients of writeToDirectory() will always get a URL back if they need it.

• … that's actually kinda it, really, if you want to keep the existing API as-is. The tiny amount of stdout logging that goes on isn't that harmful.

Have a think and let me know your decision. I'm happy to build this out and do all the docs etc once a concrete decision is made, and I'd love to be able to use this in my projects.