tesis-dynaware / graph-editor

Eclipse Public License 1.0
132 stars 42 forks source link

Added styleable properties for Grid & replaced grid lines with Canvas. #5

Closed eckig closed 9 years ago

eckig commented 9 years ago

As discucces here: https://github.com/tesis-dynaware/graph-editor/issues/1 I created an intial pull request to add two new styleable properties for the grid spacing and the grid color and replaced the collection of individual lines as nodes with a single canvas element.

The zoomed-in canvas will look blurry. But I think that is another topic which we should fix seperately as the current way of zooming in is not perfect either (the grid lines will get thicker as the whole scene graph is scaled up).

rmfisher commented 9 years ago

While the styleable properties are a nice feature, I unfortunately can't merge this because of the blurry grid lines when zoomed in.

I did however incorporate your DraggableBox changes into version 1.1.0, so there are no longer border and background rectangles. Thanks for providing that.

eckig commented 9 years ago

Sorry for being picky, but I don't like your line of arguments..

Both the current zoom approach (zooming in makes the grid lines thicker, which is not as you would expect it) as my initial canvas work do not behave well when zooming. If you really want to offer the ability to zoom the graph editor, you/we should take a different approach. But that is not a reason to decline this pull request. By replacing hundreds of single nodes with a single canvas is a huge improvement!

PS: I can not see the DraggableBox changes in 1.1.0 changes?

rmfisher commented 9 years ago

That's ok, you're allowed to be picky :)

The current look of the grid-lines when zooming is preferrable to me. Having many nodes in the scene graph is not nice, I admit, but until there is an observable performance difference I won't sacrifice visual quality.

Yeah I agree the current approach to zooming is very limited. There is certainly room to implement something more heavyweight.

DraggableBox no longer has a border and background rectangle. These were moved to the default node skin so you don't need to use them if you make your own skins.

eckig commented 9 years ago

I searched the recent commits and examined the last demo, but the DraggableBox seems to be the same?

Should I provide a pull request for the CSS styleable grid color and spacing first?

rmfisher commented 9 years ago

https://github.com/tesis-dynaware/graph-editor/blob/master/api/src/main/java/de/tesis/dynaware/grapheditor/utils/DraggableBox.java

The border and background rectangles have been removed here.

Sure, yeah if you could provide that pull request that would be great. Before submitting it could you test the following scenario? This is what I had problems with when I tried styling the grid in CSS earlier:

eckig commented 9 years ago

Ah thanks, somehow I missed that commit - created the pull request, just with the grid stroke color.