search

OneZoom / OZtree

OneZoom Tree of Life Explorer
Other
91 stars 20 forks source link

High resolution rendering #896

Closed jaredkhan closed 2 days ago

jaredkhan commented 1 week ago

Resolves #891

Before

image

After

image
jrosindell commented 1 week ago

@jaredkhan looks brilliant - thanks for posting.

hyanwong commented 1 week ago

Great, shall I merge this now @lentinj ?

davidebbo commented 1 week ago

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.

davidebbo commented 1 week ago

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.

davidebbo commented 1 week ago

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! 😃)

lentinj commented 1 week ago

Hmmm, for some reason I don't see this now! Maybe it has to get into that state somehow.

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 largest_visible_node / https://github.com/OneZoom/OZtree/issues/547 tripping you up. "Visible" in this case may just be the invisible corner of the bounding box.

davidebbo commented 1 week ago

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 largest_visible_node / #547 tripping you up. "Visible" in this case may just be the invisible corner of the bounding box.

Ah, good to know. Then let's definitely ignore it in the context of this change.

davidebbo commented 1 week ago

@lentinj is there anything needed before we can get his one approved? I think @hyanwong is waiting for this to kick off a new release. Thanks!

lentinj commented 1 week ago

@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.

davidebbo commented 1 week ago

@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! 😄

jaredkhan commented 3 days ago

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.

hyanwong commented 2 days ago

I'm keen to merge this: do you think it's ready @lentinj? There currently seems to me a conflict in renderer.js though?

davidebbo commented 2 days ago

Indeed there is a conflict. As far as I can see, refresh_by_image is not used, and probably doesn't need to be in this PR. Right @jaredkhan ?

jaredkhan commented 2 days ago

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?

davidebbo commented 2 days ago

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.

hyanwong commented 2 days ago

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.

hyanwong commented 2 days ago

(but could you fix the merge conflict before I merge, @jaredkhan? - thanks)

jaredkhan commented 2 days ago

Conflicts resolved @hyanwong

hyanwong commented 2 days ago

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.

jaredkhan commented 2 days ago

Wicked, thanks for the feedback everyone. New extinct tree is looking great with all those images!