Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Terminal enableFancyFunctionality gets disabled when processing is too slow (like a tab is in the background), but never gets re-enabled #1862

Closed
pyjamasam opened this issue Apr 11, 2017 · 17 comments

Comments

Projects
None yet
5 participants
@pyjamasam
Copy link

commented Apr 11, 2017

What were you doing?

Printing with the UI open and in a background tab

What did you expect to happen and what happened instead?

Expected result: Terminal is switched to non fancy mode due to performance decrease in background tabs. Once tab is brought to the foreground the Terminal is switched back to fancy mode due to performance returning to normal.

Branch & Commit or Version of OctoPrint

OctoPrint 1.3.2 (master branch)

Operating System running OctoPrint

Raspberry PI 2B

Linux octoprint 3.18.11-v7+ #781 SMP PREEMPT Tue Apr 21 18:07:59 BST 2015 armv7l GNU/Linux

PRETTY_NAME="Raspbian GNU/Linux 7 (wheezy)"
NAME="Raspbian GNU/Linux"
VERSION="7 (wheezy)"

Printer model & used firmware incl. version

Micromake D1, Azteeg X5 Mini 1, latest edge of smoothieware

Browser and Version of Browser, Operating System running Browser

macOS Chrome 57.0.2987.133 (64-bit)

Link to octoprint.log

I don't think I need to include this as its a UI bug

Link to contents of terminal tab or serial.log

Same as above. Not needed for a UI bug

Link to contents of Javascript console in the browser

N/A

Screenshot(s) or video(s) showing the problem:

N/A

I have read the FAQ.

Just adding this issue here as we spoke about it a little while back on IRC, but I wanted to make sure it got logged so it didn't get forgotten (and I haven't noticed a commit to reflect it being fixed - so this is also to be used as a reminder :-) )

Performance is never evaluated again after fancyFunctionality is disabled for the terminal. So once it gets disabled it never gets re-enabled till you reload the entire interface.

And at least under chrome when the Octoprint tab is in the background it often gets dinged performance wise so the processing time of log messages while printing some times drops below the acceptable limit. But when the tab is back in the foreground performance increases again to well within acceptable limits, but sadly the terminal is still in dumb mode :-(

@GitIssueBot

This comment has been minimized.

Copy link
Collaborator

commented Apr 11, 2017

Hi @pyjamasam,

It looks like there is some information missing from your bug report that will be needed in order to solve the problem. Read the Contribution Guidelines which will provide you with a template to fill out here so that your bug report is ready to be investigated (I promise I'll go away then too!).

If you did not intend to report a bug but wanted to request a feature or brain storm about some kind of development, please take special note of the title format to use as described in the Contribution Guidelines.

Please do not abuse the bug tracker as a support forum - if you have a question or otherwise need some kind of help or support refer to the Mailinglist or the G+ Community instead of here.

Also make sure you are at the right place - this is the bug tracker of the official version of OctoPrint, not the Raspberry Pi image OctoPi nor any unbundled third party OctoPrint plugins or unofficial versions. Make sure too that you have read through the Frequently Asked Questions and searched the existing tickets for your problem - try multiple search terms please.

I'm marking this one now as needing some more information. Please understand that if you do not provide that information within the next two weeks (until 2017-04-25 16:00 UTC) I'll close this ticket so it doesn't clutter the bug tracker. This is nothing personal, so please just be considerate and help the maintainers solve this problem quickly by following the guidelines linked above. Remember, the less time the devs have to spend running after information on tickets, the more time they have to actually solve problems and add awesome new features. Thank you!

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being, so don't expect any replies from me :) Your ticket is read by humans too, I'm just not one of them.

@pyjamasam

This comment has been minimized.

Copy link
Author

commented Apr 11, 2017

Updated original ticket so the bot is happier.

@foosel

This comment has been minimized.

Copy link
Owner

commented Apr 11, 2017

Ah, perfect. While I now remember the conversation, thanks for adding a ticket, because of course I had forgotten about the issue again.

foosel added a commit that referenced this issue Apr 20, 2017

Recover terminal functionality on processing speed increase
If double the speed needed for output during print/fancy terminal
functionality is detected, re-enable that after a timeout of 5s.
Decrease of speed cancels the timeout again. That should prevent wild
toggling.

Should solve #1862
@foosel

This comment has been minimized.

Copy link
Owner

commented Apr 20, 2017

Should be fixed now on maintenance (see above commit) and soon devel, will go into 1.3.3.

Difficult to test though, I wasn't able to get my clients slow enough without using breakpoints in the javascript. Any additional "real world" tests against the maintenance branch are definitely welcome ;)

@foosel foosel added this to the 1.3.3 milestone Apr 20, 2017

@pyjamasam

This comment has been minimized.

Copy link
Author

commented Apr 20, 2017

I'll give it a whirl. I was able to see it fairly reliably with my configuration.

@pyjamasam

This comment has been minimized.

Copy link
Author

commented Apr 24, 2017

So far so good. Seams to flip back and forth as needed. haven't tried it during printing, but just having it spit out 105's is enough to see it.

A good way to test also it so throw a breakpoint into the log message processing stack somewhere. Just sit on it for a second and then continue. Then remove (or disable the breakpoint).

Temporarily causes log processing to take a while and then return to normal speed.

pyjamasam referenced this issue Apr 24, 2017

BillyBlaze
Improve terminal performance by adding FastForEach
Move the AutoScroll function to an animation frame to prevent layout thrashing and to keep it in sync with fastForEach
Remove the <br> tag from the terminal and use display: block; on span tags to prevent increasing DOM nodes and faster processing of the terminal
@pyjamasam

This comment has been minimized.

Copy link
Author

commented Apr 26, 2017

Ya after playing, this seems to have fixed the tab performance issues, but now auto scrolling is broken (as referenced in that commit I commented on) in that branch.

@foosel

This comment has been minimized.

Copy link
Owner

commented May 2, 2017

@pyjamasam I was out of a commission for the past week. Just now took a look (using the same breakpoint approach that you described, which I actually also used during development testing), but I can't reproduce the "scrolling stops" behaviour right now over here. Did this only happen during actual printing or did you also repro this with the breakpoint approach?

@pyjamasam

This comment has been minimized.

Copy link
Author

commented May 5, 2017

No worries @foosel.

So the funny terminal behaviour some times manifests its self as the auto scrolling not working, and sometimes it just ends up being slightly "off" when scrolling.
Of course when I tried to actually replicate the issue I saw the strange off scrolling and not the autoscrolling being disabled.
https://youtu.be/7rJ3xeefALI
You can see how the scroll bar isn't actually at the bottom of the element, and the bottom log line is really close to being cut off.
I do have a personal plugin that just adds some CSS to make the terminal larger and the font smaller, but otherwise doesn't change anything else. The issue is also visible (in my opinion actually looks worse) with the stock styles though.
https://youtu.be/JkRA2Tqi4vk
The bottom logline it very much cut off when using the default styles.

The other part of this is that if I add some debug logging as the first line in TerminalViewModel.scrollToEnd it doesn't ever get called.

Once I rename scrollToEnd to scrollToEnd2 (and update the calling methods to reference this new name) then I see the debug logging as well as better auto scrolling.
https://youtu.be/FwDCCoEjjlc
This looks like I'd expect it to when auto scrolling is active. (and what I remember from previous releases)

I am running commit 844494a now as well.

Hope that helps.

@BillyBlaze

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2017

Thanks for the info @pyjamasam

Do you perhaps have any 3rd party plugins installed that could possible overwrite the scrollToEnd function?

@pyjamasam

This comment has been minimized.

Copy link
Author

commented May 9, 2017

My apologies for not catching that. Yes it looks like I do have something that overwrites the terminal's scrollToEnd method.

Autoscroll (0.0.2)
https://github.com/MoonshineSG/OctoPrint-Autoscroll

https://github.com/MoonshineSG/OctoPrint-Autoscroll/blob/master/octoprint_autoscroll/static/js/autoscroll.js#L18

I can confirm disabling that plugin makes the issue go away.
Sorry about that.

Only thing I have noticed now (and it might be a side effect of using the background animation stuff to do the scrolling is that if I click on another chrome tab and leave octoprint in an inactive tab with the terminal visible for a little bit and then click back to octoprint's tab the auto scrolling hasn't happened and the terminal text is off the bottom. As soon as another log message comes in the auto scrolling is done and the display looks correct. Should never be an issue when priting due to the speed at which log messages come in. But I notice it when just getting M105 requests and responses when idle.

I don't think its anything that needs to be fixed. Just an observation.

I'll open a ticket on the autoscroll repo to deal with this as its whats broke.

I think this ticket can be closed as the original issue has been corrected. (well I guess close it when the release is released :-) )

@BillyBlaze

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2017

No problem, we thought this commit would have a low impact, but it seems it is actually has a very high impact. Also based on another problem @CapnBry found. We decided to revert this commit made by me and keep on using the old foreach binding.

@foosel

This comment has been minimized.

Copy link
Owner

commented May 10, 2017

Yep, all reverted now. Good that this was identified as troublesome though before the first RC went out, less release overhead this way ;)

@MoonshineSG

This comment has been minimized.

Copy link

commented May 11, 2017

I fixed the plugin (even if the OctoPrint change has now been reverted) and should not be causing issues if the function gets updated in the future ...
sorry about causing the mess...

@foosel

This comment has been minimized.

Copy link
Owner

commented May 11, 2017

@MoonshineSG I wouldn't call it a mess - it was only a development version that was affected and while it caused some head scratching, it did that to a limited number of people :) No harm done, and the change causing the issue caused other problems as well.

@MoonshineSG

This comment has been minimized.

Copy link

commented May 11, 2017

ya, the beauty of having people willing to live on the edge... glad it was caught early.

@foosel

This comment has been minimized.

Copy link
Owner

commented May 31, 2017

Oh, I should have closed this a while ago I think :)

@foosel foosel closed this May 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.