search

Deep-Symmetry / afterglow

A live-coding lighting controller, building on the Open Lighting Architecture with Clojure and bits of Overtone.
Eclipse Public License 2.0
417 stars 23 forks source link

Translating qlc+ fixture definitions fails #72

Open BlackYps opened 2 years ago

BlackYps commented 2 years ago

Describe the bug Trying to translate a qlc+ fixture with the jar fails

To Reproduce I executed java -jar afterglow.jar -q Cameo-ROOT-PAR-6.qxf I can give you the qxf file, but I think it isn't the fault of the particular file.

Log [user@manjaro fixtures]$ java -jar afterglow.jar -q Cameo-ROOT-PAR-6.qxf Warning: environ value /usr/lib/jvm/java-16-openjdk/ for key :java-home has been overwritten with /usr/lib/jvm/java-11-openjdk WARNING: An illegal reflective access operation has occurred WARNING: Illegal reflective access by clojure.lang.InjectedInvoker/0x000000080022a040 (file:/home/user/.qlcplus/fixtures/afterglow.jar) to method com.sun.org.apache.xerces.internal.jaxp.SAXParserImpl.parse(org.xml.sax.InputSource,org.xml.sax.helpers.DefaultHandler) WARNING: Please consider reporting this to the maintainers of clojure.lang.InjectedInvoker/0x000000080022a040 WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations WARNING: All illegal access operations will be denied in a future release Exception in thread "main" java.lang.NullPointerException at clojure.zip$node.invokeStatic(zip.clj:67) at clojure.zip$branchQMARK.invokeStatic(zip.clj:73) at clojure.zip$branchQMARK.invoke(zip.clj:69) at clojure.data.zip.xml$attr.invokeStatic(xml.clj:20) at clojure.data.zip.xml$attr.invoke(xml.clj:17) at afterglow.fixtures.qxf$qxf_channelGT_map.invokeStatic(qxf.clj:166) at afterglow.fixtures.qxf$qxf_channelGT_map.invoke(qxf.clj:159) at clojure.core$mapv$fn8468.invoke(core.clj:6914) at clojure.core.protocols$fn8181.invokeStatic(protocols.clj:168) at clojure.core.protocols$fn8181.invoke(protocols.clj:124) at clojure.core.protocols$fn8136$G81318145.invoke(protocols.clj:19) at clojure.core.protocols$seq_reduce.invokeStatic(protocols.clj:31) at clojure.core.protocols$fn8168.invokeStatic(protocols.clj:75) at clojure.core.protocols$fn8168.invoke(protocols.clj:75) at clojure.core.protocols$fn8110$G8105__8123.invoke(protocols.clj:13) at clojure.core$reduce.invokeStatic(core.clj:6830) at clojure.core$mapv.invokeStatic(core.clj:6905) at clojure.core$mapv.invoke(core.clj:6905) at afterglow.fixtures.qxf$parse_qxf.invokeStatic(qxf.clj:298) at afterglow.fixtures.qxf$parse_qxf.invoke(qxf.clj:285) at afterglow.fixtures.qxf$convert_qxf.invokeStatic(qxf.clj:315) at afterglow.fixtures.qxf$convert_qxf.invoke(qxf.clj:308) at afterglow.core$_main.invokeStatic(core.clj:270) at afterglow.core$_main.doInvoke(core.clj:260) at clojure.lang.RestFn.applyTo(RestFn.java:137) at afterglow.core.main(Unknown Source)

Desktop (please complete the following information):

brunchboy commented 2 years ago

I wonder if this has to do with some of the dependencies I updated last night? But I won’t be able to check until I have a spare moment at work (over lunch, probably) and I don’t have any QXF files handy there, so attaching it here would be helpful. Otherwise I can try to find some when I get back home tonight, but either way it would be nice to try with the exact file.

And I am suspecting it may be something about the file itself not matching the expectations I had for the format because of where it is failing: https://github.com/Deep-Symmetry/afterglow/blob/16dce77149e83e6a61eff577b571c5ab2cdd8ecf/src/afterglow/fixtures/qxf.clj#L166

brunchboy commented 2 years ago

(I did steal some time to try it with some of my existing QXF files before running out the door, and it is working for them, including Cameo-Multi-PAR-3.qxf, so we are going to have to figure out what is problematic about this specific file.)

BlackYps commented 2 years ago

Cameo-ROOT-PAR-6.qxf.txt Here is the file, I had to add .txt to upload it here. Normally that isn't present. Maybe it errors out because of my lackluster setup of the fixture definition. I only partially implemented all capabilities of the fixture. I will use the occasion and fill out the rest and will see if it works then or the error persists.

BlackYps commented 2 years ago

I also tried it with this fixture definition and got the exact same error: https://github.com/mcallegari/qlcplus/blob/master/resources/fixtures/Cameo/Cameo-Multi-PAR-3.qxf

brunchboy commented 2 years ago

Thank you for sharing those files. I ended up being far too busy at work and after yesterday to take a look at them, but I did quickly this morning. The good news is that I can reproduce the problem with those files. The bad news is that comparing the Multi Par 3 file with the version from an older release of QLC+ that I have is that the reason seems to be that QLC+ has stopped including the information in the files that Afterglow found useful in building its own fixture definitions. So it may no longer be practical to try importing files from QLC+. Here is the older version of the file so you can compare for yourself. Cameo-Multi-PAR-3.qxf.txt If you can see a way to extract the same information reliably from the new file, a pull request to qxf.clj would be most welcome. We would also need some way of recognizing which version of the file format we are dealing with. Perhaps you can find some documentation of the file format and the reason for the changes, but I don’t have much hope of that, not many people create very thorough documentation for their open source projects.

brunchboy commented 2 years ago

In the mean time, this weekend I will try to remember to add a warning to the project read me that QLC+ import is currently not working due to changes in QLC+.

BlackYps commented 2 years ago

I got some good news and bad news. The good news is that it is easy to restore the missing information even in newer QLC+ versions. It seems like at some point the developer introduced presets. Switching them back to custom definitions in the fixture editor restores the info you need in the file and is also quick to do for the user. If you have a preset in a channel and switch it back to custom it already has the correct information selected, so you can change a file in less then a minute. After I have done that, when I do a file comparison I can see that some names have changed. This seems to be the reason why it still fails to convert. However, the stack trace is now less informative, so I can't really track down the assumptions you make in the closure code that are not fulfilled.

Exception in thread "main" java.lang.NullPointerException
        at afterglow.fixtures.qxf$convert_qxf.invokeStatic(qxf.clj:319)
        at afterglow.fixtures.qxf$convert_qxf.invoke(qxf.clj:308)
        at afterglow.core$_main.invokeStatic(core.clj:270)
        at afterglow.core$_main.doInvoke(core.clj:260)
        at clojure.lang.RestFn.applyTo(RestFn.java:137)
        at afterglow.core.main(Unknown Source)

With my limited clojure knowledge I am stuck here for the moment.

You can have a look yourself with this file: Cameo-Multi-PAR-3-only-customs.qxf.txt

brunchboy commented 2 years ago

Ok, that’s strange. The line that stack trace is failing on suggests that the problem is with finding the parent directory of the file that you are having Afterglow process, not with parsing the .qxf file. And indeed, when I try converting the file you attached in your most recent update, Afterglow was successfully able to translate it (compressed so that GitHub will let me post it): multi-par-3.clj.zip

Ah ha! We have now found a different bug in my QXF converter. I tried putting the .qxf file in the same folder as Afterglow, and cd-ing into that directory so I could use exactly the same command line that you were using, and that caused the same problem. I will investigate how to fix that, but in the mean time, as a workaround, if you use an absolute path with the -q argument, you should have better luck converting files, for example:

java -jar afterglow.jar -q /Users/james.elliott/Desktop/Cameo-Multi-PAR-3-only-customs.qxf
brunchboy commented 2 years ago

There may well be new or different information in the QXF files that Afterglow can take advantage of them, so if you have time to study them as you learn about what goes into an Afterglow fixture definition, and see anything that might seem useful, either try to extend my converter, or tell me about it and I can help.

brunchboy commented 2 years ago

All right, I fixed that problem, you should now be able to convert files in the current working directory with relative paths if you download the latest preview jar from https://github.com/Deep-Symmetry/afterglow/releases/tag/v0.2.6-SNAPSHOT

brunchboy commented 2 years ago

To close this issue, we’ll need to update the instructions on how to convert a fixture to use custom channels instead of prefixes. However, it may also be possible to simply support the prefixes, which would make life easier for everyone. This page in the online manual has some information about it, but we will probably need to explore the user interface to figure out what all the presets are and what they translate to. I will probably take the step of just updating my own user guide to tell everyone to convert channels to custom, but exploring the available presets and how to translate them would be a nice starter project for a new contributor. Either way, I should download the new QLC+ and see exactly how you did convert a fixture to work, so I can describe it properly.

brunchboy commented 2 years ago

Unfortunately, while our furniture was being delivered on Saturday, a wall sconce globe was broken, and researching how to get that replaced, as well as giving away our old furniture, consumed the time I hoped to work on this. Perhaps next weekend will treat me better.

BlackYps commented 2 years ago

I can confirm that the 0.2.6 version now works with relative paths. When providing an absolute path I also managed to translate the fixture definition with the 0.2.5 version. I will report back if I run into more issues or if I have ideas for improving the converter. This may take a while though, because work is now claiming more time again.