Spade-Editor / Spade

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

Refactor core classes #76

Closed HeroesGrave closed 9 years ago

HeroesGrave commented 10 years ago

Progress Check

BurntPizza commented 10 years ago

I'm working out a plan for the actual drawing area: https://github.com/BurntPizza/Paint.JAVA/commit/11734b85436f1b9e7c1818d5d602bdff762a6ba5

That part of the codebase is the one I'm least satisfied with, and one of the most integral to the application as a whole.

Note that it's heavily WIP, but I wanted to get it out and let you know what I'm thinking. Please comment on the commit if you have ideas/comments.

HeroesGrave commented 10 years ago

I'm thinking along the lines of the following:

Document is the base of everything. It contains the layers, the selection, and the history. It will handle the drawing surface, or at least the class which handles the drawing surface if we need the extra abstraction.

Layer is just a cleanup and renaming of Canvas.

Selection will still be based of of Layer (Canvas), but a bit of cleanup will be required, as well as a better method of masking.

One problem we have is the history. What I really want to do is to extract it from the Canvas/Layer class and have it all togther as part of the document. However, this leads to problems when you need to reconstruct the layers for rendering. I don't like this, but it seems the current situation (History+Global History) is the only way around this issue.

BurntPizza commented 10 years ago

Some things in the current model I don't like/understand: (note that these are not slams on your work, please don't take it like that)

What exactly does Canvas do? I understand it keeps track of the current image in the work area, plus some history management, which is all fine and dandy, if it was self-contained, which it is not: it does half of the overall job, leaving the other half to CanvasRenderer etc, which makes no sense to me. Plus, many of them are created, seemingly for many different uses which use it for random chunks of functionality it contains.

Why do IFrames have Canvases? What even are IFrames/Frames/KeyFrames? They seem to be used for all kinds of things which don't seem to relate to each other, plus I dislike the enormous quantity of BufferedImages being created, drawn, and passed around all the time, it's a huge waste of resources.

Why is Selection a canvas? Shouldn't selection be a state of a canvas, not a canvas itself?

Same with Layer(s), I think it should be a stateful aspect of a canvas, as you only draw to one canvas, however you may draw to a specific layer in that canvas and move layers around, etc.

My Canvas proposal is pretty much your Document, but includes Layers and Selection, as I think those are aspects of a (the) canvas. It may very well control history as well, if deemed applicable. (I think it is, as everything else is going through the same place)

I don't think there should be History vs. GlobalHistory, my idea is that there is one Canvas/Document, with one history. The user only ever does one thing at a time, why should there be multiple histories? If layers are a concern, just have MergeChange, LayerMoveChange, ect. Also other changes could either specify which layer the action was performed on, or the history could store silent ContextChange events, signalling which layer is selected and thus is being acted upon. It reminds me of OpenGL texture binds in that sense. Continue in a similar fashion for SelectionChange, etc.

In my head, all of this goes on like a well oiled machine, with little object allocation being performed (those BufferedImages/Frames kill, I tell ya), and minimal rendering occurring, only when it needs to. I'll try to make that a reality inside the testtube of my experimental package, or at least enough of it so you know what I'm talking about, in implementation detail; sometimes code is just a better communication medium.

HeroesGrave commented 10 years ago

The mess on the canvasmanager/canvas classes is due to the leftovers from the time we only had a single layer.

The reason for the current canvas-has-history thing is that the changes that depended on Graphics2D could not be easily reverted, so we had to wind back and replay the changes instead. One of the main things I'm doing in the refactor is moving away from Graphics2D so we don't have to worry about that.

This is going to be a big job, so I'm not expecting to make much progress until this weekend. If things go well, I reckon we'll be able to continue normal work by next week, if not sooner.

BurntPizza commented 10 years ago

One of the main things I'm doing in the refactor is moving away from Graphics2D

How so do you mean? Moving away as in not depending on it for shape rendering? I agree that is a good idea. However you make it sound like there won't be a need for a Stack or similar to "wind back and replay," but I don't see a way around it. Could you explain how you see Changes being handled by Canvas/Document?

I'm currently diagramming my idea here: Paint.JAVA Canvas - Draw.io

HeroesGrave commented 10 years ago

The canvas needs to handle changes because when you undo a change, it rewinds back to the last keyframe and replays all except the one that gets removed. Because keyframes are specific to each layer (or canvas), the canvas must store its changes.

Moving away as in not depending on it for shape rendering?

Yes. Graphics2D rendered shapes are inconsistent and hard to serialise.

There are three options as far as how we undo/redo changes:

  1. Every pixel change stores the new state. This means to change things we have to rewind and replay because we don't store the old state.
  2. Every pixel change stores both it's old and new state. This makes changing things really simple, but takes up 2x as much memory.
  3. Every pixel change stores the non-current state. What I mean is when the change is applied, the pixel change stores the old state, and when it is reverted, it stores the new state. This seems like the best solution, but it feels a little bit hacky and might cause problems if the history doesn't line up (because pixel-changes will be applying the wrong colours).

Despite the problem with idea 3, I still think it is the best solution. It would also allow for decoupling the history and canvas/layer.

Another aspect I think needs changing is a way of indexing the layer tree as a flat array so changes can easily find which layer they were applied to. Again, it is a good gain in terms of efficiency, but like idea 3 for the last point, it might cause problems if the history doesn't line up (Because layers will have different indexes).

However, I think as long as we are 100% certain that the history will line up, it will work.

Apart from that, my one concern is that without Graphics2D, all our work is being done in the software layer instead of being hardware-accelerated. If this causes problems, I think the only solution is to go into some native code for the most intensive parts (eg: image blending). I really want to avoid bringing in native dependencies, but if things start to go really slow, we may have to.

BurntPizza commented 10 years ago

I understand the keyframe business, if you look at my diagram I have them implemented as "Freeze Buffers" (named after Frozen Tracks in Abelton Live) where a pile of changes (or one large intensive change) are "frozen" to a backbuffer, which is then used to render those changes while the user makes new changes on top. The difference is that they exist entirely in the Canvas, the rest of the app doesn't know about them, as they're really just hacks to allow us to not have to render everything all the time, not actual workspace state changes.

Pixel-by-pixel storage of changes is a recipe for disaster with performance, as storing every pixel changed by an operation means that you then have to process every single one sequentially, and it becomes hard to make guarantees about the data which can be reliably exploited for speed.

My approach would be more or less how it's already done: the history (Canvas) keeps a Stack where each change is more or less an independent renderable entity. To make a change (user drew a circle) push a Change (of a type relevant to the action performed by the user) onto the stack. These are rendered via the command pattern where each Change has a inherited draw(...) method. To undo a change, pop the stack and repaint. So to combine that with the FreezeBuffers, instead of rendering from the most recent keyframe up (the current system), we render the freezebuffer, then the stack of most recent changes. When a change gets old enough (or is large enough, this is detectable by timing the draw() method) it is merged with the freezeBuffer. The history would manage things similarly with serializing old changes to the disk. The proposed render stack does implement the layers as a flat list, sorted by their index.

As long as expensive (again, cost can be monitored realtime) changes are frozen to buffers, and the only things going on are either the blitting of those buffers or cheap PixelChangeSomethings, I think performance should be good. Image blending is completely paralleliziable, so there's that as well. Just grab the pixel data arrays, divide them among threads, and compute the output array. I did a silly micro-benchmark performing that exact action here: http://www.java-gaming.org/topics/multithreaded-arithmetic-benchmark/32494/msg/303920/view.html Also in my diagram I take layer blending into account by freezing all layers that are not the currently active layer. Since the user can't do anything (that wouldn't require a stack restructuring anyway) to them while on another layer, they can be safely frozen, thus eliminating the blending computation for all but the currently selected layer. Optimization is all about exploiting known behaviors.

I do like #3, but I don't think it's possible to save much with it as you have to repaint the canvas anyway. Tests are in order!

I have my final AP exam tomorrow, but after I get back I'll try to sit down and bang out a flow diagram just for the history (if my brain isn't too fried), it's probably going to be a bit more complicated than the render stack.

Hoo, that was a lot of text, sorry ;)

EDIT: this might be of use to us for hashing out designs: http://www.processon.com/ or http://creately.com/

HeroesGrave commented 10 years ago

Can't see your drawing. Not on Chrome.

Your solution makes sense, apart from one thing:

How do you transfer a change from the history buffer to the layer image when you go back further than the last change pushed to the freezebuffer?

Good luck with your exam :)

BurntPizza commented 10 years ago

Ah, sorry about that, here is the current draft: Imgur I'll probably move to an entirely online solution (like creately) for diagramming tomorrow as well, so that won't be a problem later.

Current idea, the most straight forward approach: (pseudocode)

public Change pop() {
    if(changeStack.isEmpty()) { // we're into the freezebuffer
        /* copy from memory (or deserialize, but that shouldn't happen as
            it should be asynchronously prefetched) a pile of changes at once */
        copyXElements(history.getStack(), changeStack, BLOCK_SIZE);
        refreeze(); // rebuild freezebuffer
    }
    return changeStack.pop();
}

That, or unfreeze the most recently edited layer in the freezebuffer, which now that I think of it is probably the better way to go. Maybe. I'm tired. Layers will probably have internal freezebuffers as well, so it shouldn't be too bad.

Again, this should all start coming together much nicer tomorrow.

HeroesGrave commented 10 years ago

The code you posted above doesn't quite solve the problem. Because we're going back further than the freezebuffer, it means we don't have anything to build from, which means reconstruction is impossible.

I'm going to have to do a bit of research on how some other programs do stuff like that.

HeroesGrave commented 10 years ago

Added progress check at the top of this thread.

HeroesGrave commented 10 years ago

Okay. I have the application running with no image editing. A good start.

BurntPizza commented 10 years ago

The code you posted above doesn't quite solve the problem. Because we're going back further than the freezebuffer, it means we don't have anything to build from, which means reconstruction is impossible.

Writing changes to the freezebuffer doesn't eliminate them from the history, they're still there, and perfectly reconstruct-able.

Also, pondering "Change-mode No. 3," having the changes store the information they overwrote (to allow easy undo) will probably not perform well, as that data takes approx. numPixelsChanged * (bitdepth+locations) space to store, which would become massive very quickly. I still vote that changes merely would the information required to write themselves. (location, shape, etc.)

I just pushed a new branch to my fork: experimental, that's where I'll be doing the new rendering engine overhaul, at least for now. I'm thinking we abstract Graphics2D with a RenderContext, see here: https://github.com/BurntPizza/Paint.JAVA/commit/cd8d8f7563b2529caf34b7cd63c022324dbcd6df

Idea is that the RenderContext's implementation is independent (it's an interface), so we can easily use our own drawing algorithms or even image representation transparently under the hood.

HeroesGrave commented 10 years ago

The issue I was pointing out was that we need a way to reconstruct the image without going all the way back in the history.

I think that keyframes are going to have to stay, but we may need to refine them a bit.

HeroesGrave commented 10 years ago

Er... this is awkward.

I'm working on this at the same time, and I think our approaches are different.

I'm trying to make sure that things like the history and rendering methods are easy to swap out, so we might be able to integrate both.

HeroesGrave commented 10 years ago

I've come across a problem with your idea of freezing layers.

Blending layers is usually done bottom-up to preserve ordering and consistency. (Like constructing a building by placing each layer of bricks on top of the next)

However, the idea of freezing all the layers above introduces top-down blending. (Like constructing a building by holding up the roof while you cement bricks to the bottom of it)

Also, there is a consistency problem in that depending on which layer you're editing, the layers may be blended in a different order. (Using the previous analogies, this is like pre-constructing several layers of bricks, lifting it into the air at it's desired location, and then cementing bricks underneath that.)

These analogies are getting ridiculous, but hopefully you can see my point.

A possible solution is to make freezing work in a bottom-up way. This means that the layers below the edited one can all be frozen together, while the ones above have to be redrawn every time.

This may result in some confusing code, but the result will be more intuitive.

BurntPizza commented 10 years ago

Sorry, internet been down since ~20 hours ago >_>

Yea, I planned on dividing layers between frozen together and laying on top of the currently edited layer.

Should I just work on refactoring other stuff for now? (like the colorchooser, that's a giant bowl of spaghetti)

HeroesGrave commented 10 years ago

Yeah, the colour chooser could do with a cleanup.

If you want, could you make it so the slider panel can be exchanged for a different panel and hidden completely?

BurntPizza commented 10 years ago

Progress on ColorChooser: the pallet selector panel

image Shown is the pallet generated by Pallet.defaultPallet().

Not sure I'm completely happy with the dashed lines, but they accomplish their job of being visible over all backgrounds.

No object allocation required while painting it (at least in my code, not sure about stdLib), so that's good.

HeroesGrave commented 10 years ago

Nice

BurntPizza commented 10 years ago

More progress: (ignore GIF quantization) palletpanel gif

HeroesGrave commented 10 years ago

That is amazing. (Maybe open a new issue though. This thread is long enough as is)

BurntPizza commented 10 years ago

Ok, but one more before I call it a night!

slider

Not fully implemented yet (no hsv sliders), but I'll do that tomorrow. Still no allocations, even with changing gradients! Mwuahaha!

sylvia43 commented 10 years ago

@BurntPizza Quick comment on the pallet selector: The box around the current box and the box moused over look exactly the same, which can be confusing. It looks pretty legit though :+1:

HeroesGrave commented 10 years ago

I got the pencil tool working very well.

Here's a look at the change behind it: https://gist.github.com/HeroesGrave/5db591a3ac87dcf2a528.

For now I'll keep using Graphics2D here, but if I start seeing inaccuracies, I'll just swap it out with a proper line-drawing algorithm.

I've also started working on the History system, but it isn't working quite yet.

(The serialization methods are there, but not used. I'm not quite sure if they work yet)

HeroesGrave commented 10 years ago

Ugh. Just realised I have exams in about a week and an important project for computing due around the same time.

I'll still be able to do work on this, but it'll take a bit longer than originally expected.

Longor1996 commented 10 years ago

I am working on a better CanvasRenderer, and I got to say that it works pretty well after only 5 hours of coding:

I didn't put the scrollbars back in yet, since I will probably have to code these in manually (make them from scratch) because of the rotation thing. Why do we need the scrollbars actually? I think they are pretty useless.

Have a picture: newpaintcanvas

Edit: I forgot to read all the comments. Seems like BurntPizza is already working on making the CanvasRenderer better. Though I still think it would be nice to 'rotate' the Canvas.

BurntPizza commented 10 years ago

I'd like to see your code for this, maybe push an experimental branch on your repo?

Longor1996 commented 10 years ago

Its only a single class (which is wrapped into my 'Testomatiko' project, where I test things), so I can quickly put it into a gist. I didn't check directly how good performance is, but it seems like there are pretty much no memory issues.

Edit: Uploaded the Gist, note the notes at the beginning of the file. https://gist.github.com/Longor1996/6a3f70c0cb125c4c8274

HeroesGrave commented 10 years ago

That looks really good.

When it's finished, make a pull request so I can merge and integrate it before things get too messy.

Longor1996 commented 10 years ago

I will probably just go and modify the current renderer to support all the nice things, although its probably complicated enough that rewriting the entire renderer is a much better idea.

My biggest problem with the renderer right now is (which caused me to attempt rewriting it), that the renderer is embedded into a bunch of JComponent's (which makes things slow and complicated), instead of being just a single component outfitted solely for the task of displaying a image.

Tl:Dr; Lets 'collapse' all the components required for displaying the image into one thing that handles all of it in a much more efficient way.

BurntPizza commented 10 years ago

Lets 'collapse' all the components required for displaying the image into one thing that handles all of it in a much more efficient way.

That is what I have wanted as well, glad we seem to agree on that.

HeroesGrave commented 10 years ago

I will probably just go and modify the current renderer to support all the nice things, although its probably complicated enough that rewriting the entire renderer is a much better idea.

I've already scrapped the current renderer so it's definitely better to rewrite.

Longor1996 commented 10 years ago

I am about halfway done with the 'PaintCanvas' (as I call it), the things missing are:

HeroesGrave commented 10 years ago

Scrollbars are so you can navigate the image while zoomed in.

On my side, I've got the basic history working very well (I think). Changes are being serialised as they get old, and deserialised when you undo too far back. The thresholds for each function are easily customizable.

BurntPizza's idea of 'FreezeBuffers' is implemented and AFAIK, it's working well. I am still yet to test it on a more intensive scale.

One thing I haven't done yet is have progressive keyframes in the freezebuffers. If you have 1000 changes and then revert 1, it has to redraw the previous 999.

I think I'll put off writing changes to the disk for a little while, and get things working well enough to push to an experimental branch.

I'm still using BufferedImages and Graphics2D because I don't want to interfere too much with what you guys are working on.

BurntPizza commented 10 years ago

Scroll bars can be replaced with grab-pan functionality which is what Longor's Canvas is doing (middle-click and drag to pan around). This is actually more intuitive IMHO.

I'd like to see some of the code, push somewhere when you're at a good point.

HeroesGrave commented 10 years ago

@BurntPizza: Here's the link to the branch. Most of the work (or at least the interesting parts) is going on in that folder.

Be aware that nearly all functionality is broken. This is just for showing what the code looks like so far. Obviously quite a bit will change between now and when I merge the code in.

BurntPizza commented 10 years ago

Ok so I got it running (back under Swing, don't have time for Web LAF) amazing it runs with 80% of packages uncompliable lol.

I see your still using ScrollPanes, maybe it works better under WebLAF, but zooming is still glitched to hell. I'm going to try and drop in Longor's renderer tomorrow (it's very late) and see how that goes.

Undo/redo seems to pretty instant (only testing pencil, but still) so that's good, although previews are (will be) more intensive.

HeroesGrave commented 10 years ago

Oh yeah. Forgot to mention that lots of things were removed from the build path when I stripped down everything.

My one concern with all the freezing of changes and layers is that we end up with 3 BufferedImages per layer. 2 for the freezebuffer (before the changes and after the changes), and a third to store the layer and all its children predrawn.

Depending on how well the new rendering pipeline goes, we might be able to cut out the 3rd bufferedimage, or at least only create it for images with more than a certain number of children.

HeroesGrave commented 10 years ago

Just tried zooming again and found that yes, it is extremely glitched. I must've broke something recently because it was working fine before.

I think I'll leave it for now because Longor's PaintCanvas is going to replace the current system anyway.

BurntPizza commented 10 years ago

I think having a few images per layer is ok, as I think speed is of more concern in the Renderer than raw memory usage, and those are a constant memory usage, not active allocations (read: steady and not much). Also I may be able to figure out some trickery to avoid having them for each layer, we'll see.

HeroesGrave commented 10 years ago

I/O working again. (Except LBIN/ZLBIN, because layers aren't functioning yet)

Longor1996 commented 10 years ago

Just a quick comment (Got to go back to tradeschool in a few minutes):

To make the PaintCanvas work correctly, ALL the tools which depend on transforming the mouse-position into a pixel-position in the image, WILL have to go trough a given 'transformScreenToImage(Point2D in):Point2D'-method.

On another note: I will put in some code to make it possible for tools to draw things on top of the image as they are being used. This should allow things like 'splines' to be implemented, as they are in the Paint.NET 'Line'-tool. You know, these fancy dots which you can drag around when you made a new line? Yep, these.

BurntPizza commented 10 years ago

Hey, maybe this isn't the most appropriate place for this, but I got bored and wrote a little tool that could be useful while refactoring this project (or any project): LagMeter

timer

Should be good when working out canvas rendering, etc. Thought you guys might like it. Could even include it in the project, hidden as a debug feature. (and yes, I got the idea from Minecraft's debug screen)

HeroesGrave commented 10 years ago

Looks smooth.

May as well have it as a hidden debug feature.

BurntPizza commented 10 years ago

It's much smoother actually, the gif kinda ruins it (capped at 33 fps I believe). Unless you meant smooth as in "Hey, that's pretty smooth there man" :P

Yeah, might as well, useful for any potential plugin developers.

Longor1996 commented 10 years ago

Why not go and put in the same thing, but for 'Memory' instead of 'Time'? Then make it switch between the two on the press of a button or something like that.

BurntPizza commented 10 years ago

I believe there's already something like that: hereosgrave.paint.gui.MemoryWatcher Don't remember what it looks like though.

Longor1996 commented 10 years ago

The MemoryWatcher is my doing, I coded it up. I think it would be better/look better, if the MemoryWatcher was 'integrated' into the LagMeter.

BurntPizza commented 10 years ago

You can go ahead and add whatever to it, but I'm keeping the basic timing caliper functionality in it's own tool. I limited the scope of it on purpose.

In other news: I've been working on the RenderContext stuff, implementing some actual algos, and it's going quite well performance wise, even large, across the screen brush strokes render in ~1/20 of a millisecond (!) including alpha blending. Stay tuned.