OctoPrint
GCodeViewer: Async download
- [x] Your changes are not possible to do through a plugin and relevant to a large audience (ideally all users of OctoPrint)
- [x] If your changes are large or otherwise disruptive: You have made sure your changes don't interfere with current development by talking it through with the maintainers, e.g. through a Brainstorming ticket
- [x] Your PR targets OctoPrint's devel branch if it's a completely new feature, or maintenance if it's a bug fix or improvement of existing functionality for the current stable version (no PRs against master or anything else please)
- [x] Your PR was opened from a custom branch on your repository (no PRs from your version of master, maintenance or devel please), e.g. dev/my_new_feature or fix/my_bugfix
- [ ] Your PR only contains relevant changes: no unrelated files, no dead code, ideally only one commit - rebase and squash your PR if necessary!
- [x] Your changes follow the existing coding style
- [x] If your changes include style sheets: You have modified the .less source files, not the .css files (those are generated with lessc)
- [x] You have tested your changes (please state how!) - ideally you have added unit tests
- [ ] You have run the existing unit tests against your changes and nothing broke
- [x] You have added yourself to the AUTHORS.md file :)
What does this PR do and why is it necessary?
This PR moves the download of the GCode file in the GCodeViewer to the Worker and makes it asynchronous. The GCode file is never entirely in memory.
This now allows downloading a 500MB GCode file in Chrome (with compression enabled).
How was it tested? How can it be tested by the reviewer?
Downloaded and parsed several files.
Any background context you want to provide?
What are the relevant tickets if any?
Screenshots (if appropriate)
Further notes
This is not complete, it does not implement the skipUntil feature yet.
The pre-commit hooks fail on eslint, because it does not handle async functions correctly.
I do not know how to fix that.
I have made the PR anyway so this can be evaluated in the mean time.
The pre-commit hooks fail on eslint, because it does not handle async functions correctly.
I do not know how to fix that.
This is intentional as eslint is set up to enforce ES5 compatibility (many people out there are still using old tablets with ancient browsers that don't support ES6). So no async keyword support and no arrow functions either. See also a9ea150b927db7d0a275ece9103218eab3e9bfa2. With nodejs-bin it might be possible to give an initial babel run a chance here, but that is something that should be looked at outside of the context of this PR.
@foosel I don't think I can implement this without those keywords. Do you have statistics of your octoprint clients? ES6 is already 7 years old...
Good point actually, the last time this came up we did not yet have data on this, so around the time I added the linter to prevent accidents after a complaint I also added the client environment to the tracking data:
https://data.octoprint.org/export/client_environment_stats_30d.json
Alright, that are some useful statistics.
| Type | number | percentage |
|---|---|---|
| Total Instances: | 175974 | |
| ES6 Support: | 174438 | 99.13% |
| No Support: | 399 | 0.23% |
| Unknown support: | 1137 | 0.65% |
This is the script that calculated the statistics. If you can identify which versions of WebKit support ES6 you can get 424 extra instances classified. convertstatistics.js.txt
With that data, a case could be made for dropping ES5 support. However, that is not a decision I cannot make overnight (and while exhausted AF). I've got a vacation coming up, I'll look into this again on my return at the end of July. I've extracted it as a request into #4562. In there I also mention this:
If we go for this, what we definitely should make available/link to is some page for OctoPrint's users to figure out if their browser supports ES6 (and possibly other stuff in the future). caniuse.com has the data, and https://caniuse.com/es6 holds the info on ES6 specifically, but I couldn't yet locate a "you are here" kinda feature. Any hints in that regard would be highly appreciated.
Any input on that point would be greatly appreciated.
I've just pushed a commit to maintenance that stops the enforcement of ES5. ES6 language features such as arrow functions and async/await should thus no longer be an issue to get through the pre-commit, and are hereby also allowed.
This does not include the skipUntil feature because that is difficult. It needs a decision on if and how this needs to be implemented. Statistics on who actually uses this feature would be useful.
Since we are parsing as the data comes in, we need to skip the data until the "SkipUntil" string. If this string is not found, it will not parse any data. This would be a change in functionality compared to the previous functionality when the setting was ignored if the string was not found.
The only way to implement the old functionality would be to cache all data while searching for the string, throwing it away if it is found or parse it all when it isn't. In the case the string isn't found, we are back to caching the whole gcode file in memory. Considering avoiding this is exactly why this PR exists, it seems contra-productive to me.
Hmm, I thought of a possible solution.
If the generator function would work as normal but the string check would be part of parsing a gcode line, we could purge any data that has been parsed so far, basically resetting the parsing. This would include a command sent to the reader to purge any layers it has received this far.
The generator would also check for the string and return it as normal but reset the currentDownloadLength and adjust the totalDownloadLength variables.
It will need some serious documention, however.
Statistics on who actually uses this feature would be useful.
Sadly I have absolutely zero data on that, sorry.
Am I right in understanding that the problem is that we do not know whether skipUntilThis is even contained in the file or not? If so, how about some server side pre-check in that regard? One request that utilizes something like this (which I'd be happy to move into OctoPrint's core utils) to figure this out, then the viewer can make an informed decision on how to proceed, without any overhead. If for some reason the request or check cannot be made, assume no skipuntil value was set and just visualize everything.
Yes, that is the issue. If we know for sure it is in the file, we can throw away the data until we see the string. A server side check would simplify the gcode viewer a lot.
Btw, based on #4594, now we have to implement this feature in the worker, we could modify the current code to only ignore head movements and extrusion commands but parse all the other stuff normally. This would mean a change in how the feature works but it would prevent such issues.
Btw, how is the percentage handled on server side? If the gcode viewer cuts off a part of the file, the completion percentage calculation is diffferent from the one on the server and things should get out of sync?
we could modify the current code to only ignore head movements and extrusion commands but parse all the other stuff normally
Probably a good idea, I say go for it.
Btw, how is the percentage handled on server side? If the gcode viewer cuts off a part of the file, the completion percentage calculation is diffferent from the one on the server and things should get out of sync?
The viewer calculates the percentages based on position in file vs file size, before the skipUntil string ever gets even remotely looked at. So they do sync up, the viewer side just skips the first few percent.
Probably a good idea, I say go for it.
Will do.
The viewer calculates the percentages based on position in file vs file size, before the skipUntil string ever gets even remotely looked at. So they do sync up, the viewer side just skips the first few percent.
I don't think this is accurate. The first thing the gcodeviewer does is shorten the file, before it is parsed and the percentages are calculated. See https://github.com/OctoPrint/OctoPrint/blob/maintenance/src/octoprint/plugins/gcodeviewer/static/js/gcodeviewer.js#L515 which calls self.showGCodeViewer which does the skipUntil split and passes the shorted file to the gCodeReader which calls prepareGCode which does the percentage calculation. So unless Octoprint itself also calculates the percentages on a shorter file, they get out of sync.
Sorry if I'm missing something, but I've been unable to load any gcode in the viewer with this commit applied. The in-UI status is stuck on "Parsing... (100%)" and the browser console displays:
09:20:53.786 Error calling fromCurrentData on view model GcodeViewModel : loadFile@http://127.0.0.1:5000/static/webassets/packed_core.js?cc63d943:798:523
GcodeViewModel/self.loadFile@http://127.0.0.1:5000/static/webassets/packed_core.js?cc63d943:727:866
GcodeViewModel/self._processData@http://127.0.0.1:5000/static/webassets/packed_core.js?cc63d943:731:740
GcodeViewModel/self.fromCurrentData@http://127.0.0.1:5000/static/webassets/packed_core.js?cc63d943:728:448
callViewModelIf@http://127.0.0.1:5000/static/webassets/packed_core.js?cc63d943:1726:75
callViewModelsIf/<@http://127.0.0.1:5000/static/webassets/packed_core.js?cc63d943:1720:153
Pn@http://127.0.0.1:5000/static/webassets/packed_libs.js?6bee1dee:30:530
m/ur/<@http://127.0.0.1:5000/static/webassets/packed_libs.js?6bee1dee:51:66
callViewModelsIf@http://127.0.0.1:5000/static/webassets/packed_core.js?cc63d943:1720:95
callViewModels@http://127.0.0.1:5000/static/webassets/packed_core.js?cc63d943:1719:72
DataUpdater/self._onCurrentData/<@http://127.0.0.1:5000/static/webassets/packed_core.js?cc63d943:1600:266
DataUpdater/self._ifInitialized@http://127.0.0.1:5000/static/webassets/packed_core.js?cc63d943:1614:1002
DataUpdater/self._onCurrentData@http://127.0.0.1:5000/static/webassets/packed_core.js?cc63d943:1600:226
OctoPrintSocketClient.prototype.propagateMessage/<@http://127.0.0.1:5000/static/webassets/packed_client.js?39d41c31:24:116
Pn@http://127.0.0.1:5000/static/webassets/packed_libs.js?6bee1dee:30:530
m/ur/<@http://127.0.0.1:5000/static/webassets/packed_libs.js?6bee1dee:51:66
OctoPrintSocketClient.prototype.propagateMessage@http://127.0.0.1:5000/static/webassets/packed_client.js?39d41c31:24:77
onMessage/<@http://127.0.0.1:5000/static/webassets/packed_client.js?39d41c31:31:316
m/Zt/<@http://127.0.0.1:5000/static/webassets/packed_libs.js?6bee1dee:47:23
_t@http://127.0.0.1:5000/static/webassets/packed_libs.js?6bee1dee:35:392
m/Vt/<@http://127.0.0.1:5000/static/webassets/packed_libs.js?6bee1dee:46:369
m/ur/<@http://127.0.0.1:5000/static/webassets/packed_libs.js?6bee1dee:51:73
onMessage@http://127.0.0.1:5000/static/webassets/packed_client.js?39d41c31:31:278
[5]</r.prototype.dispatchEvent@http://127.0.0.1:5000/static/webassets/packed_libs.js?6bee1dee:1585:2864
[14]</</w.prototype._transportMessage/<@http://127.0.0.1:5000/static/webassets/packed_libs.js?6bee1dee:1585:13481
[14]</</w.prototype._transportMessage@http://127.0.0.1:5000/static/webassets/packed_libs.js?6bee1dee:1585:13432
[3]</i.prototype.emit@http://127.0.0.1:5000/static/webassets/packed_libs.js?6bee1dee:1585:1745
[38]</c/this.ws.onmessage@http://127.0.0.1:5000/static/webassets/packed_libs.js?6bee1dee:1585:39734
packed_core.js:1727:37
Though I haven't yet been able to figure out what the error it's reporting actually is. Tried in Firefox and Chrome, in multiple environments (systeminfo bundle is from a minimal dev environment in upstream/maintenance, but also tried switching my "normal" home environment to maintenance). Multiple gcode files tried, and I've bisected it directly to this commit.
octoprint-systeminfo-20221027092154.zip
Bundles:
edited by @github-actions to add bundle viewer links
I can confirm I wasn't able to load a gcode file - chromium based browsers give much more helpful error messages (at least for OctoPrint):
Error calling onTabChange on view model GcodeViewModel : ReferenceError: totalSize is not defined
at Object.loadFile (http://localhost:5000/plugin/gcodeviewer/static/js/viewer/reader.js:217:65)
at GcodeViewModel.self.loadFile (http://localhost:5000/plugin/gcodeviewer/static/js/gcodeviewer.js:524:35)
at GcodeViewModel.self.onTabChange (http://localhost:5000/plugin/gcodeviewer/static/js/gcodeviewer.js:875:22)
at callViewModelIf (http://localhost:5000/static/js/app/helpers.js:1455:35)
at http://localhost:5000/static/js/app/helpers.js:1382:13
at Pn (http://localhost:5000/static/js/lib/lodash.min.js:9:530)
at Function.<anonymous> (http://localhost:5000/static/js/lib/lodash.min.js:30:66)
at callViewModelsIf (http://localhost:5000/static/js/app/helpers.js:1380:7)
at callViewModels (http://localhost:5000/static/js/app/helpers.js:1374:5)
at OctoPrint.coreui.exports.onTabChange (http://localhost:5000/static/js/app/main.js:153:13)
Error calling fromCurrentData on view model GcodeViewModel : ReferenceError: totalSize is not defined
at Object.loadFile (http://localhost:5000/plugin/gcodeviewer/static/js/viewer/reader.js:217:65)
at GcodeViewModel.self.loadFile (http://localhost:5000/plugin/gcodeviewer/static/js/gcodeviewer.js:524:35)
at GcodeViewModel.self._processData (http://localhost:5000/plugin/gcodeviewer/static/js/gcodeviewer.js:633:34)
at GcodeViewModel.self.fromCurrentData (http://localhost:5000/plugin/gcodeviewer/static/js/gcodeviewer.js:552:18)
at callViewModelIf (http://localhost:5000/static/js/app/helpers.js:1455:35)
at http://localhost:5000/static/js/app/helpers.js:1382:13
at Pn (http://localhost:5000/static/js/lib/lodash.min.js:9:530)
at Function.<anonymous> (http://localhost:5000/static/js/lib/lodash.min.js:30:66)
at callViewModelsIf (http://localhost:5000/static/js/app/helpers.js:1380:7)
at callViewModels (http://localhost:5000/static/js/app/helpers.js:1374:5)
Another way you can get more helpful error messages is to disable to bundling/minifying - see the docs (link to temporary new docs because config is better documented) https://octoprint.github.io/docs/user-guide/configuration/config-yaml/#user-guide.configuration.config-yaml.devel
