Spade-Editor / Spade

Cross-platform raster graphics editor inspired by Paint.NET
GNU General Public License v3.0
41 stars 10 forks source link

Moving Forwards Again (Discussion) #75

Closed HeroesGrave closed 9 years ago

HeroesGrave commented 10 years ago

First of all, here's the things we need to work on before 1.0:

Please comment with any thoughts or discussion you may have.

Edit: The big changes are going to be part of #76

HeroesGrave commented 10 years ago

Reopened #69 due to the colour picker still being a bit glitchy.

HeroesGrave commented 10 years ago

Also, feel free to comment in any issue that you want to work on and I'll assign you.

BurntPizza commented 10 years ago

My main issue right now is the convoluted nature of the core code, it was obstructing my attempts at fixing a lot of things. I say it needs a big ol' re-factoring. For instance: GUIManager, Canvas, CanvasManager, CanvasRenderer, too many classes all doing each other's jobs; makes it very hard to change anything atomically without breaking everything around it.

I can draw up some simple plans on possible class architecture, I'll see what I can do.

HeroesGrave commented 10 years ago

Opened #76 (Refactor core classes).

Longor1996 commented 10 years ago

If we are going to roll our own 'Graphics2D' to modify the images, we can actually go and stop using BufferedImage alltogether (except for rendering), and make our own little 'Image'-class, that gives us direct access to the pixels, thus speeding up everything (a lot), and keeping the memory clean of garbage.

A class like:

(pseudocode)
class ImageARGB
{
    // Raw Pixels encoded as ARGB.
    int[] pixels = ...;
    int width = ...;
    int height = ...;

    void setPixel(int x, int y, int argb)
    {
        out-of-bounds-checks-here
        pixels[x + y * width] = argb;
    }

    void setPixelDirect(int x, int y, int argb)
    {
        NO out-of-bounds-checks-here
        pixels[x + y * width] = argb;
    }

    int getPixel(int x, int y)
    {
        out-of-bounds-checks-here
        return pixels[x + y * width];
    }

    int getPixelDirect(int x, int y)
    {
        NO out-of-bounds-checks-here
        return pixels[x + y * width];
    }

    static ImageARGB mixImage(ImageARGB left, ImageARGB right, ImageComposite composite, ImageARGB target)
    {
      // mix the images together, using the given alpha rule, put the result into 'target'.
      return target;
    }

    BufferedImage convertToAWTImage(BufferedImage target)
    {
         // check if ImageARGB and BufferedImage have the same size
         target.setRGB( ..., pixels);
         return img;
    }

    etc.etc.etc
}
BurntPizza commented 10 years ago

That's more or less what I had in mind with RenderContext, except it still has a Graphics2D/BufferedImage/WritableRaster/whatever underneath. pixels[] would most likely still be present via

((DataBufferInt) buffer.getRaster().getDataBuffer()).getData();
Longor1996 commented 10 years ago

Wasn't there a problem with the memory synchronization of Java when you directl access a BufferedImage's 'DataBuffer'? As far as i am concerned, misplacing a single line of code while handling the DataBuffer directly can crash performance into the Earth's core.

I already had that experience with my raytracer. Got really angry at the syncronization issues and finally replaced the AWT output with a direct OpenGL output, ONLY for performance's sake. There was a difference of 300 FPS between the AWT and OpenGL display.

BurntPizza commented 10 years ago

My experience has always been that direct pixel access is by far the fastest method of manipulating a bufferedimage pixel-by-pixel. I'll go find an old benchmark of mine...

Longor1996 commented 10 years ago

Its pretty damn rare that a performancee crash happens, but while making a RayTracer this happens quite often for some reason. Probably because of the constant writing to the DataBuffer, which happens `800_600_60' time per second.

BurntPizza commented 10 years ago

Here's a test of drawLine() vs setRGB() vs int[] vs byte[] on bufferedimages: https://gist.github.com/BurntPizza/a7ae9e324ad3c6eb1137 It's not the best, as I exploited some things to speed it up, but it's still making a valid comparison between int[] and byte[] for sure. It also gives a general overview of performance for the other methods.

Also tested and verified that Arrays.fill() and System.arraycopy() are lightning fast and do work with both byte[] and int[].

EDIT: biggest thing I've found to increase performance is removing unpredictable branching from the render logic, even a single ?: operator per pixel murders performance compared to a branchless design when it's of an unpredictable manner and the branch predictor can't optimize it.

Longor1996 commented 10 years ago

Your benchmark gave me these results: Rendering Test: 1000 frames... Time for drawLine(): 96.97 ms/frame Time for setRGB(): 23.57 ms/frame Time for int[]: 9.58 ms/frame Time for byte[]: 10.31 ms/frame

Seems like the int[]-draw is the fastest of them. I will repeat the benchmark now with Thread-Priority = MAX and NUM_FRAMES = 10'000. Edit: I had to stop the benchmark, it took WAY too long.

BurntPizza commented 10 years ago

With NUM_FRAMES that large you may want to comment out the drawLines test, it will take a while.

My results on a Phenom II 955:

Rendering Test: 1000 frames...
Time for drawLine(): 47.75 ms/frame
Time for setRGB(): 12.63 ms/frame
Time for int[]: 4.79 ms/frame
Time for byte[]: 4.86 ms/frame

Ratios between methods are more or less the same as yours. What hardware are you using?

Longor1996 commented 10 years ago

My Hardware (not the best):

BurntPizza commented 10 years ago

That's very similar to mine (Phenom 955 + HD 4670 (old I know)), interesting how mine is about twice as fast...

BurntPizza commented 10 years ago

Another bench, this time for filling rectangles: https://gist.github.com/BurntPizza/7a5b93d3eaa9f910c8c6 It draws (fills) 100 randomly sized rectangles, each of which is either green or blue (again random) per frame.

I could not get setRGB() to work, but I doubt it would have performed well anyway. Results:

Rendering Test: 1000 frames...
Time for fillRect(): 5.57 ms/frame
Time for int[]: 3.91 ms/frame
Time for byte[]: 2.86 ms/frame
Longor1996 commented 10 years ago

Quickly tested it, here are the results:

Rendering Test: 1000 frames...
Time for drawLine(): 5.88 ms/frame
Time for int[]: 4.03 ms/frame
Time for byte[]: 3.98 ms/frame

PS: Did you look at the renderer (the one with rotation)?

BurntPizza commented 10 years ago

Yeah the renderer looks pretty good as the V part of MVC (also some of the C), but we still need a fast M, supposedly Heroes is reworking stuff for that.

Longor1996 commented 10 years ago

So... we will need to come up with a 'M' (Model) that can do pretty much all the stuff Graphics2D can do, but faster/fast enough to be used with our own system?

I think my idea to make a completely custom 'Image'-class (and a custom 'Graphics'-API) is pretty good, if done right.

By the way: Trying to make our own system by making it a sort of 'implementation' under the Graphics2D-interface is kind of dumb, because there are so many methods to re-code that it borders on insanity (How many interface methods does Graphics2D have...? Edit: 83!).

BurntPizza commented 10 years ago

The custom graphics is part of it, but also how the canvas handles layers and changes when rendering. See the conversation between Heroes and I for more about that.

I didn't mean to write an implementation for the actual Graphics2D (or even Graphics), but merely an implementation of a subset of their features, i.e. the drawing primitives we use (drawLine, drawRect, drawPixel, etc.). See https://github.com/BurntPizza/Paint.JAVA/blob/experimental/src/experimental/canvas/context/WritableRenderContext.java for a simple example (although lacking impl details)

HeroesGrave commented 10 years ago

So you're saying rolling our own image class is more efficient? Great. That makes things a lot simpler for me. :+1:

My one concern is how efficently we can convert the image into a BufferedImage for rendering. (On that note, is there another way to render to a Swing component?)

BurntPizza commented 10 years ago

My one concern is how efficently we can convert the image into a BufferedImage for rendering.

My thought is that the image is a wrapper on a BufferedImage (similar to the original Canvas class) that simply also wraps Graphics stuff that we need. Then there is no "conversion," only a getBackingImage() (or whatever) when you're ready to give it to swing.

HeroesGrave commented 10 years ago

What about implementing RenderedImage? All they require is access to a Raster, and they can be drawn with Graphics2D#drawRenderedImage()

According to your tests, accessing the Rasters was pretty fast.

BurntPizza commented 10 years ago

That would work, but it'd be more work than just having a class (that doesn't implement any stdlib interfaces) and just using

g.drawImage(context.getImage(), 0, 0, null);

I don't think we need to go too deep here, implementing Image or RenderedImage or subclassing BufferedImage from scratch probably would result in the best performance, but simply wrapping a bufferedimage won't be much slower, and requires far less wheel re-inventing.

Wrapping also allows us to easily delegate functions to the underlying image's actual Graphics, if per-pixel operations are not required/not faster for that specific function.

Longor1996 commented 10 years ago

There is a model ('M') in my head, and it got messed up somehow in this discussion, so...

@BurntPizza can you make a pseudocode version of how you imagine the 'custom image class'? I am kind of confused as to what the goals/requirements/ideas of the 'custim image class' actually are right now. Care to explain, please?

BurntPizza commented 10 years ago

Current prototype, some impl, some left out: https://gist.github.com/BurntPizza/5699893ec9ce26c28c00 Feel free to suggest more functions that you anticipate will be needed.

Longor1996 commented 10 years ago

Looking good! But I would recommend replacing the 'bresenham'-thing in the 'fillRect'-method with the same thing that is used in 'clearRect'. It would be way more efficient.

Also, here is an idea: To support arbitraty shapes (like: Rotated Rectangle, Triangles, etc.etc. using AffineTransform), someone should code up a scanline-rasterizer that works directly on top of the BufferedImage. Just an idea, though.

BurntPizza commented 10 years ago

Unfortunately all pixel modifications that don't completely overwrite the buffer (clearRect) have to go through drawPixel in the end, to keep alpha blending. That's why I've made several revisions to the gist speeding up drawPixel as much as I can.

As long as a shape is composed of lines, it wouldn't be hard to rotate efficiently. I'll look into it.

Longor1996 commented 10 years ago

Well, by making a scanline rasterizer (or vector rasterizer), one can make the whole thing even easier. I will explain that later. No time right now!

HeroesGrave commented 10 years ago

I've not been having a great week.

On top of getting ready for some exams next week, I also have a computing/tech project due next week worth about 1/3 of this year's mark. To make things worse, I've caught a cold and am struggling to focus on work (whether schoolwork or programming) for much more than 20 minutes.

I'm not trying to make excuses (they are perfectly valid anyway), but I thought I'd better let you know why I can't get much done at the moment.

On the plus side, I have several days free over the next week or so due to public holidays and days where I have no exams.

BurntPizza commented 10 years ago

Sorry to hear that, get your rest though! I now have plenty of free time (I'm on summer break), so just tell me what I can work on that won't interfere with what you're currently doing and I'll do that.

I'm writing a wiki page (because I can cleanly present the idea in that format) for how the plugin API could work, should I do that and then just start implementing it after you look at the spec?

HeroesGrave commented 10 years ago

The plugin system is probably the only thing that isn't going to be affected by the upcoming changes.

Feel free to come up with whatever you can for the plugin system, and implement it if you wish. (If you want to discuss something, there's #51). Chances are that even some of the craziest ideas will have a valid use case or two.

BurntPizza commented 10 years ago

Alright, I'll see what I can do. I'm thinking something along the lines of how libGDX does it, but I'm not sure if a single "Gdx" class with a bunch of static methods is the best way to expose the API. It is sure easy to use/do though...