firionus / opengdtf Goto Github PK

• Parse GDTF XML with roxmltree
• Metadata
• Geometries
  • Change from indextree to petgraph, we'll need it later anyway for ModeMaster dependencies
  • Completely rework first implementation. Important points:
    • Top-level Geometries must be parsed first so GeometryReferences to them can be linked on the fly
    • Put in a "fullPath" or "fullName" or "geometryPath" or "geometryNode" containing the way it is referenced inside GDTF. Or should this be a method?
      • But the name of a geometry must be globally unique anyway, so what's the use? Let's not do it.
  • What if a top-level Geometry is a GeometryReference? Can there be nested GeometryReference chains?
  • Consider validation at each single step
  • References
  • Break_Offsets (Handling of Break Overwrite?)
• Figure out whether parse-function should be TryFrom method
  • No, because it takes any Read + Seek-argument, which is not semantically strong enough (is it zipped or not?, etc.)
• Add a clearer status message to README
• Remove GDTF files from GDTF-share, as they don’t have any license and the person creating them or even GDTF-share might have an issue with it (also delete on all old commits). Replace with a folder where the developer has to manually place fixture files (is API ready?) and smoke tests are run. Results should be manually inspectable, as well. Maybe include an automatically generated list of recommended fixtures, with file names, versions, source URL (manual and optional) and file hashes. Provide a way to assert certain results for certain files or to do snapshot testing. If we want to run specific tests, we ought to replicate ourselves the cases in the files, creating more minimal and maintainable test cases as well.
• Fix broken tests
• Replace Vec<HandledProblem> with a custom type alias
• Empty string for attribute RefFT should be parsed as "None" silently. This is against the DIN (the attribute doesn't have a default value and is therefore assumed required), but semantically it is clearly optional and the GDTF Builder always outputs an empty string instead of omitting the attribute. Throwing a problem every time seems unnecessary.
• Review GDTF Example Tests (variable names are messy)
• Write documentation on GDTF Example Tetsts in CONTRIBUTING.md
• Deterministic Geometry Deduplication
• Review Code: Refactor, Add Tests, ...
  • Review parser::geometries.rs
  • Check validity of everything, also against DIN
  • Check things against our own principles
  • Lint to disallow expect and unwrap outside tests?
  • The code should clearly separate between domain logic (building Gdtf, validating it upon modifications, ...) and parsing logic. That will make it much easier later to offer a modifiable and serializable API.
  • When building a struct from a document, we need to be lenient. Example: We wouldn't require Unique Geometry Names globally, only between children of each top-level geometry and between top-level geometries and all non-top-level-geometries. We break these duplications with UUID insertion, as we do already. However, what if someone references a Geometry, expecting to reference the one limited to the top-level geometry of the current DMXMode, but because we renamed it with a UUID, that now points to the wrong Geometry, pointing outside the geometry of the DMXMode, breaking that reference. How do we deal with this? Special casing UUID-renamed Geometries during parsing? We could keep a list of renames during the parsing...
  • So basically, when we later resolve Geometry references, we first look normally, then check if it is a descendant of the right top-level geomery (we have to do that anway), if yes alright, if no look in a list of parsing renames: (top-level geometry, old_name, new_name). If old_name occurs multiple times for the same top-level geometry, error the Geometry reference. Otherwise, resolve the reference to the geometry with new_name.
  • But when building a Gdtf struct via the API or serializing it, we want the strict definition of "Unique Geometry Names" to be obeyed.
• Update Deps
• Remove TODOs
• Fix Clippy Warnings about Panics in code
• Format all files and enable "format on save"
• Move hash_gdtf into main library (own module). Can we replace all that logic with the CRC-32 inside the zip? -> almost, still need to combine filename and CRC32 for each file and hash that (but it's way smaller) (benchmark that it's actually faster)
• Add Docstrings to functions with some explanations, this will make it easier to later remember what a function does
• Warn on Array Indexing, since it might panic. Is there a way to write error handling code with get that does not result in massive code blocks of Problem::Unexpected?
• Write a really simple benchmark against gdtf_parser, just to check we're not magnitudes slower already ;)
  • It's an acceptable factor 1.5 (see 645dc70)
  • Interestingly, with the release build opengdtf is (at this point, we barely do any work) still slightly faster:

$ cargo run --release --bin comparison_against_gdtf_parser
    Finished release [optimized] target(s) in 0.06s
     Running `target/release/comparison_against_gdtf_parser`
parsing 156 example files with both crates and measuring time...
gdtf-parser successfully parsed 155 fixtures
opengdtf successfully parsed 155 fixtures
gdtf-parser took 0.48860416 s
opengdtf took 0.44242147 s
opengdtf relative time to gdtf-parser: 0.9054803181142704

• Make gdtf.name of type Name

• it's not enough to just produce a list of output channels, though that should be calculable with a method
• orient along https://github.com/OpenLightingProject/open-fixture-library/blob/master/docs/fixture-format.md
• I already wrote a Kotlin implementation in https://github.com/cueglow/glowdtf
• e.g. separate "normal channels" from "template channels" (I really like the name, compared to "abstract channel" - that's what you get from programming on JVM 😂 )
• Once Channel Layout is done: look at example channel_layout_test fixture with "missing_break_in_geometry_reference". In a Geometry Reference, it misses a DMX Break for Break 2. It's not clear what should happen to me now, but there should definitely be a problem. (What did glowdtf do?)

So working with the library as I built it is sometimes pretty cumbersome. Some reasons:

• Parsing needs lots of context for good error reporting
• All basic validation, cross-linking, etc. happens in one huge chunk of code

A better way might be:

• First parse with local validation (probably hand-written, pull-based with quick-xml)
• Intermediate validation (e.g. uniqueness of names in their containers, etc.)
• Then do cross-linking of all the referential stuff
• Intermediate validation
• Here ends the two-way compat
• Instantiate templates
• Final validations

Problem: How to know the file location in later validation steps? -> Either attach a file location to everything, or use roxmltree and attach an original node reference to everything with which we can later retrieve the position. Or stick with the current architecture, but it is one-way only and hard to later extend to be two-way.

Althought it seems like the first step could be done with quick-xml serde-derive, it (probably?) wouldn't support the lenient error handling of opengdtf. I think we'd have to write our own serde or quick-xml code.

But this might come with its own issues. Just wanted to write down that thought.