High resolution rendering #896
Conversation
|
@jaredkhan looks brilliant - thanks for posting. |
|
Great, shall I merge this now @lentinj ? |
|
Let's hold off, I found a bad issue. It works great on my external monitor, but on my laptop's monitor, it starts flashing like crazy and the site is essentially unusable. If you can't repro, let's work together to try to figure this out. |
| @@ -122,15 +122,17 @@ class Controller { | |||
| return; | |||
| } | |||
| call_hook("before_draw"); | |||
| if ((tree_state.widthres != this.canvas.clientWidth)||(tree_state.heightres != this.canvas.clientHeight)) | |||
| if ((tree_state.widthres != this.canvas.clientWidth) | |||
| ||(tree_state.widthres * window.devicePixelRatio != this.canvas.width) | |||
There was a problem hiding this comment.
If window.devicePixelRatio is not an integer (e.g. it's 2.5 in my case), you can end up with tree_state.widthres * window.devicePixelRatio not being an integer, so it will never match this.canvas.width, which gets rounded. This causes the canvas to be set up continually, causing endless flashing.
There was a problem hiding this comment.
Flooring it fixes it:
if ((tree_state.widthres != this.canvas.clientWidth)
|| (Math.floor(tree_state.widthres * window.devicePixelRatio) != this.canvas.width)
|| (tree_state.heightres != this.canvas.clientHeight)
|| (Math.floor(tree_state.heightres * window.devicePixelRatio) != this.canvas.height)) {There was a problem hiding this comment.
Ah great spot David thanks for this!
Have pushed that change throughout.
|
I found a small regression: with the new code, I need to zoom in significantly more on a leaf before it makes it to the URL. It doesn't happen until the leaf fills the entire screen, which is not something that would normally happen. |
Hmmm, for some reason I don't see this now! Maybe it has to get into that state somehow. If no one else sees this, maybe we can ignore it (but I did see it! 😃) |
You're not making this up! This is definitely possible, but I'd be very surprised if it was a related regression. This is most likely |
Ah, good to know. Then let's definitely ignore it in the context of this change. |
|
@davidebbo glancing at the code it looks good to me, but I'd like to play with it first if there's time. I've tried to merge the 2 chunks of canvas-size-setting-code in the past and failed, if I can remember why it'd be good to see if it's still an issue. |
|
@lentinj Yep, makes sense to take the time to play with it based on your previous experience. I'm just eager as once I played with it on my big 4K monitor, there is no going back! 😄 |
|
Fixed a regression with the 'life_expert' page's screenshot tool where it was not clipping the result correctly. Just applied the same scale factor to that. |
|
I'm keen to merge this: do you think it's ready @lentinj? There currently seems to me a conflict in |
|
Indeed there is a conflict. As far as I can see, |
|
Yeah that’s right @davidebbo. That function (which required updates in this PR) was removed by another PR that was merged recently. So just scrap that function now. Re merging this: can always revert it if there is something glaring that we’re missing? |
Yes. Also, we can start by putting this in the extinct tree which has a much smaller audience. And maybe the beta site? Then if we don't see issues after a few days of playing with it, we can pull it in the main site. |
|
Definitely we can merge it and test on the extinct tree and then beta.onezoom.org too. I'm happy to press "merge" if we all thing this is generally fine. |
|
(but could you fix the merge conflict before I merge, @jaredkhan? - thanks) |
|
Conflicts resolved @hyanwong |
|
Thanks so much for this @jaredkhan . Such great work. It is now merged and is the version running the extinct tree at https://www.onezoom.org/extinct/life/ if you want to try it out. |
|
Wicked, thanks for the feedback everyone. New extinct tree is looking great with all those images! |
Resolves #891
Before
After
