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

Fixed image spacing error for the preview #69

Merged
merged 2 commits into from
Dec 12, 2019
Merged

Conversation

ajusa
Copy link
Contributor

@ajusa ajusa commented Dec 11, 2019

This pull request is a tentative fix for #68
The issue described an off by one error in the way the images were rendered. After doing a lot of debugging and testing, I realized that this actually wasn't the case. The issue was that the images were very large, and there was padding to the left of the images due to the table in HTML.

To fix this, I added some CSS rules to space out all of the images so that they fit on the screen. While this doesn't fix the issue of the padding on the left, it is unclear what the correct behavior should be. Based on the preexisting table headings, I am assuming that the spacing on the left is intentional and should be left as is.

Fixed version:
Screenshot from 2019-12-10 21-28-46

So note the spacing on the left is due to the table heading from above the image. My fix now allows all of the previews to be displayed, which made it look like an off by one error when it really wasn't.

Full screenshot:
Screenshot from 2019-12-10 21-30-12

The large amount of vertical space is part of the original Minecraft schematic, so I wasn't able to do much about that. I would propose we delete the space above the actual build changes, if it is empty. I will look into this in a later PR.

Let me know if there are any issues!

@ajusa ajusa changed the title Fix #68 FIxed image spacing error for the preview Dec 11, 2019
@ajusa ajusa changed the title FIxed image spacing error for the preview Fixed image spacing error for the preview Dec 11, 2019
@ajusa
Copy link
Contributor Author

ajusa commented Dec 11, 2019

The Travis CI build appears to be failing for a completely unrelated reason, as it mentions Lua classes I didn't touch or use directly.

@madmaxoft
Copy link
Member

Unfortunately this is a completely non-solution to the problem. True, the description in the issue is somewhat unclear, leading to the confusion.

The root of the problem is that the plugin is trying to export too large an area as the image. Instead of exporting only the bounding-box of the changed voxels, it exports the entire gallery area, including the ~180 air blocks above any build and all the way down to the bedrock layer.
The original idea for the export coords is as follows:

  1. If the area has a user-defined bounding box (as maintained in the shared DB by the GalExport plugin), use that.
  2. If not, use the stored min and max coords of the edits (each block placement and removal should update these).
  3. If not (for areas created before the edit-tracking code), calculate the diff of the area with the original area data (as set in the gallery's template schematic)
    It seems part of this decision code is missing, and the third step has the off-by-one problem when diffing.

(Updated the original issue, too)

@madmaxoft madmaxoft closed this Dec 11, 2019
@madmaxoft
Copy link
Member

You really need to download the Gallery backup and check the data from there to see the issue happening properly, and especially see the "good" case, how it is supposed to work (the GalExport-managed data works fine)

@madmaxoft
Copy link
Member

As for the CI failure, that's completely unrelated, the plugin checker seems to have some emulated APIs wrong.

@ajusa
Copy link
Contributor Author

ajusa commented Dec 11, 2019

Quick question through: This fix does stop the images from being cut off in the gallery preview, which is a different bug. Would you considering merging it just to fix that? On my laptop, I had to zoom out considerably just to see the images, as I have a low resolution display.

@madmaxoft
Copy link
Member

So how exactly does it fix it? Does it scale the images to fit? To fit what? (Sorry, the screenshots you provided are not too clear for me, I don't see a reasonable difference between "before" and "after")

@ajusa
Copy link
Contributor Author

ajusa commented Dec 11, 2019

That's probably because I never attached the before image 😄
As I thought you were referencing it in the original issue. Here is how it normally looks on my laptop:

Screenshot from 2019-12-11 11-27-24

In the other images I posted earlier, you can see how the images fit into my browsers viewport, whereas before the fix I end up with this cropped image that I can't scroll over to. The only way I can view the image is by zooming out an unreasonable amount of the web page.

So yeah, I added some CSS code that causes the image to be restricted by the width of the parent container, which means it all fits into my browser as in the above images.

@madmaxoft
Copy link
Member

I'll try it on the Gallery server...

@madmaxoft madmaxoft merged commit d4289e8 into cuberite:master Dec 12, 2019
@madmaxoft
Copy link
Member

Looks good enough as a hotfix. Though I'm not sure why there's no horizontal scrolling at all, it had to be possible before when the plugin was first written.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants