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

Larger Canvas Size Support #786

Draft
wants to merge 50 commits into
base: master
Choose a base branch
from
Draft

Conversation

IceSelkie
Copy link
Member

Primary goal is to fix #492 which caused crashes at various "large" resolutions for different people.

  • SampleBuffer class to hold the sample buffer and alpha buffer of render as double[][] instead of double[]
  • Changed BitmapImage class to use int[][] instead of int[], to be consistent with SampleBuffer
  • Made BitmapImage.data protected for better encapsulation.
  • Refactor things to work with the new SampleBuffer and BitmapImage. And use the getters and setters instead of directly accessing array (which is faster in most cases too).
  • Fixed wrong comment in FloatingPointCompressor class for decompression logic
  • PngFileWriter now takes BitmapImage instead of int[]; PfmFileWriter does similar internally.
  • BitmapImage function local variables refactored to correct names.
  • BitmapImage#vFlipped() now uses System.arraycopy and is thus much faster.

Known Issues:

  • Denoiser plugin no longer can directly access double[], and will need to be updated to use the SampleBuffer (and while changing that, may as well switch to the existing PfmFileWriter)
  • Limits "Render Preview" canvas size in RenderCanvasFx class to prevent strange JavaFx crashes. This should be changed in the future. The current state can only view the top left corner of the render. I might change this to only show the middle 4k x 4k section, but ideally we should have some zoomed out preview which isn't so big that it crashes (and doesnt take much CPU time away from actual rendering).

@NotStirred
Copy link
Member

As discussed on discord, this is a temporary solution at best, and proper render regions would be the better approach.
How much does this PR hurt performance? Some profile results would be nice on multiple setups, I'll get some results on my machine later today.
High sample counts will be the most affected most likely

@IceSelkie
Copy link
Member Author

This pull request is marked as a draft pull request, as it has a few issues with efficiency as well as compatibility. In its current state, I do not expect it to be accepted, without some decent improvements.

I believe it has done better than master in most of the test cases due to a couple slight changes in a few of the files that were made, so I will be creating a separate pull request to fix a couple of those things, should they prove to be the cause of increased efficiency.

@jackjt8
Copy link
Member

jackjt8 commented Jan 20, 2021


2.3.0-55-gad38006d0 -  3399s, 312347 SPS

max-res-fix - 421s, 314,944 SPS

2% faster

---

1920x1080 32 SPP

2.3.0-55-gad38006d0 - 435s, 304,495 SPS

max-res-fix - 421s, 314,944 SPS

3.2% faster

---

3840x2160 128 SPP

2.3.0-56-g4419bd7e - 3377s, 314,331 SPS

max-res-fix - 3273s, 324,291 SPS

3% faster

At the very least this Draft does not reduce performance.

@leMaik
Copy link
Member

leMaik commented Jan 29, 2021

@StanleyMines This PR (along with #798) is pretty much exactly what I'd need to add non-uniform sample distribution (canvas cropping, selective rendering, …). The performance seems to be slightly better than before, this PR looks pretty good.

Is there anything (other than the BitmapImage.java conflict) that would need to be changed before merging this?

Copy link
Member

@leMaik leMaik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why this shouldn't be merged. It's great! 😮

chunky/src/java/se/llbit/chunky/renderer/RenderWorker.java Outdated Show resolved Hide resolved
chunky/src/java/se/llbit/chunky/renderer/RenderWorker.java Outdated Show resolved Hide resolved
chunky/src/java/se/llbit/chunky/renderer/Renderer.java Outdated Show resolved Hide resolved
chunky/src/java/se/llbit/chunky/resources/BitmapImage.java Outdated Show resolved Hide resolved
chunky/src/java/se/llbit/chunky/ui/RenderCanvasFx.java Outdated Show resolved Hide resolved
* SampleBuffer class to hold the sample buffer and alpha buffer of render as double[][] instead of double[]
* Changed BitmapImage class to use int[][] instead of int[], to be consistant with SampleBuffer
* Made BitmapImage.data protected for better encapsulation.
* Refactor things to work with the new SampleBuffer and BitmapImage. And use the getters and setters instead of directly accessing array (which is faster in most cases too).
* Fixed wrong comment in FloatingPointCompressor class for decompression logic
* PngFileWriter now takes BitmapImage instead of int[]; PfmFileWriter does similar internally.
* BitmapImage function local variables refactored to correct names.
* BitmapImage#vFlipped() now uses System.arraycopy and is thus much faster.

Known Issues:
* Denoiser plugin no longer can directly access double[], and will need to be updated to use the SampleBuffer (and while changing that, may as well switch to the existing PfmFileWriter)
* Limits "Render Preview" canvas size in RenderCanvasFx class to prevent strange JavaFx crashes. This should be changed in the future. The current state can only view the top left corner of the render. I might change this to only show the middle 4k x 4k section, but ideally we should have some zoomed out preview which isnt so big that it crashes (and doesnt take much CPU time away from actual rendering).
Minor changes like rename and slight logic changes, and:

Now using the `int[][] spp` to store spp for each pixel; this will increase memory use a bit, but also opens the door for not rendering the whole canvas at the same time. This is not fully implemented yet, as Scene#spp is used pretty much everywhere still.
@IceSelkie IceSelkie marked this pull request as ready for review January 31, 2021 03:51
@IceSelkie
Copy link
Member Author

Un-'draft'ed this PR.

Still need to fix test cases, though I think everything else is working fine atm. (@leMaik knows more about what exactly needs to be done.)

int pixels = input.length / 3;
int size = pixels - 1;

long size = input.numberOfPixels() - 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change! Could you do the same in the decompress method?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do 👍

@IceSelkie
Copy link
Member Author

Note: Hard coded 4096x4096 display instead of full image is not sufficient. This still crashes chunky on my other computer, when using my second monitor.

@IceSelkie
Copy link
Member Author

Note: Hard coded 4096x4096 display instead of full image is not sufficient. This still crashes chunky on my other computer, when using my second monitor.

This seems to depend on the windowed size of the chunky GUI itself. When it is large (5000x3000 ish) it crashes at small sizes (2.5k by 2.5k) when window is small (900x300) it can do a bit larger (5kx5k), although I cannot seem to get larger than 5kx5k even on single display, and minimal window size, at least on this computer. (Java 11.0.3 + JavaFX LTS 11.0.2; as well as java 1.8.0u281)

@jackjt8
Copy link
Member

jackjt8 commented Feb 1, 2021

Both the UI and Canvas need access to the same Texture from what I can gather with JavaFX. The maximum supported texture size depends on the GPU, driver, and D3D/OpenGL pipeline used.

-Dprism.verbose=true should give us a quick readout of Maximum supported texture size: x which we could use as the baseline but we would also need to factor the UI size. I'm not sure about a programmatic way we can check the hardware capabilities but at least with OpenGL calling GetIntegerv(GL_MAX_TEXTURE_SIZE) would work.

@aTom3333
Copy link
Member

aTom3333 commented Feb 1, 2021

I've not followed this too closely but what I suggest is to limit the on-screen canvas to have its biggest dimension be in some interval, let's say [1000; 2000] px. You then take the biggest dimension of the real canvas and you divide it by 2 until it falls into the interval. For example, for a (imaginary) 76005100 canvas, you do the division twice to get an on-screen canvas of 19001275 which javafx supports without issue and each on-screen pixel correspond to 4*4 pixels in the real canvas, that's a round number easy to deal with.
To get the on-screen canvas from the real canvas can be done in the finalizePixel function so that it only reads 1 sample out of x (4 in this example) and write it to the backBuffer.

@leMaik
Copy link
Member

leMaik commented Feb 2, 2021

@StanleyMines I'd like to drop the changes done to BitmapImage. They are not required for anything but saving the final image (if that image has more than 2^31-5 pixels, which it will not have). I'd like to keep this as simple as possible, which would be:

  • Have the SampleBuffer at full resolution and with variable spp
  • Store number of samples of every pixel in the dump file
  • Specify area to be rendered before starting the rendering
  • Only display the area that is being rendered, show spp for that area… behave like this area is what is being rendered
  • Merge variable-spp dumps (and also merge the rendered area into it automatically after rendering, but that's already what Chunky does)

This would allow people to manually split their images into tiles and merge them, it could also be automated by automatically generating the json files (we'd need some new property like "crop": [x,y,w,h] in it) and would also enable area-based tile splitting in ChunkyCloud.


Once that's done, we can always come back to this and add a nice UI or a view for the entire image. But the most important aspect about this is to be able to split large resolution jobs into multiple smaller jobs, which is not possible at the moment.

Downscaled preview should no longer crash on 99% of computers, and will display entire render.

We should really have some way to set previewShouldSubsample, though I havent implemented that yet.

Preview render should probably also only show up to the max shown size instead of rendering far more pixels than will actually be shown. Its a preview afterall, is it not?

We should probably update preview scale %s. (And this should work great with @alexhliu's new auto-scaling preview PR)
@IceSelkie
Copy link
Member Author

IceSelkie commented Feb 4, 2021

Added Downscaleing to render previews:

  • Downscaled preview should no longer crash on 99% of computers, and will display entire render.

Fixed Merging:

  • wrong spp indices in SampleBuffer methods
  • missing spp coefficient for merge samples
  • merge tests didn't set spp for whole scene sample buffer

Future things to consider doing:

  • We should really have some way to set RenderCanvasFx#previewShouldSubsample, though I haven't implemented that yet.
  • We should probably update preview scale %s to reflect the downscaling. (And this should work great with @alexhliu's new auto-scaling preview PR, More canvas scaling controls #810 )
  • Preview render should probably also only show up to the max shown size instead of rendering far more pixels than will actually be shown. It's a quick preview after all, is it not?
  • (from leMaik's comment) 💡 In the future we could add a PreviewSampleBuffer that supports not having a sample for every pixel and uses the color of the nearest neighbor sample to create an image. That would solve Better Live Preview #736

@IceSelkie IceSelkie requested a review from leMaik February 4, 2021 02:42
Fix conflict to be able to merge into master.
@IceSelkie
Copy link
Member Author

This should fix the issue described in #492 where a single array cannot hold the whole image, however I just realized that #492 does not mention the issue of strange JavaFx crashes as I mentioned in the first comment of this PR, which is the issue that is discussed by #27 and #314. This JavaFx crash has an annoying part to it that it fails and crashes the JavaFx thread, where there is no chunky code on the stack trace at all, with no outwardly noticeable indication of the crash (other than resizing the window might lead to flickering or red boxes, which is not something that I think most people are constantly doing when waiting for canvas size to increase). Since it is entirely in JavaFx code, there isn't an easy way that we can wrap it in a try-catch and either restart it with a smaller canvas or notify the user with a warning/error dialog.

* Improved alphabetical sort by scene name to now ignore case (unless they are otherwise identical)

* Render Time column now has reasonable formatting ("4hr 07m 14s" instead of "4:7:14", or "8s" instead of "0:0:8", or "—" instead of "0:0:0")

* Changed column widths to more reasonably fit their data (and no longer have very weird numbers as widths)

* Size column now will have the crop size in brackets, if the scene is defined as a subarea in the .json.

* Renamed window "Load Chunky Scene" from "Select 3D Scene"
@IceSelkie
Copy link
Member Author

IceSelkie commented Mar 26, 2021

Updates:

  • Merging fixed in more ways than one.
  • Load Scene UI has been updated.

Things left to do in this PR:

  • All things set out to do have been accomplished. (Let me know if anything was missed)

For the future (probably not this PR):

  • Way to subdivide renders within the UI (Currently requires manual editing of the scene json file)
  • Allow compression of dumps (possibly changing order of chunks within file for better compression (deflate on SPP values))

@IceSelkie
Copy link
Member Author

@leMaik said:

Dump compression needs to come back [...]

So I guess there is (at least) one more thing to do here...

Further testing needed to make sure this actually works.
Note: this removes tests on the merging for this format. They will need to be added back eventually. Current issue is that the existing code either: 1) does not work with GZIP, or 2) doesn't test the new features of this format's merging (variable spp & cropped dumps).
Oop  -.-

(this was added a few commits ago when I was testing stuff and the end of file wasn't where I expected, and I wanted some breathing room.)
Removing 512 changes the file. Who would have guessed.
@IceSelkie
Copy link
Member Author

IntegratedSppDump now uses gzip to compress all of the dump after the header. This is slower than the floating point compressor by a bit, and is far far slower than uncompressed, but is capable of saving ~10% size. Merge testing is currently not functional for the new features though.

Next thing I plan to work on is adding some (jank) UI way to subdivide a scene and possibly move to the next subarea to render. (I'll fix dump merge test once theres a better solution to compression... atm I don't think its worth putting in more effort to get the test to pass (more than implementing the compression), if I know its working right now and its probably going to be changed very soon...)

@leMaik
Copy link
Member

leMaik commented Apr 10, 2021

@StanleyMines Sounds good 👏 Maybe do the UI on a new branch so that we can finally get this feature merged when the compression is ready

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