KarthikRIyer / swiftplot

Swift library for Data Visualization :bar_chart:
Apache License 2.0
398 stars 36 forks source link

Added single renderer function to draw Rectangles #95

Closed boronhub closed 4 years ago

boronhub commented 4 years ago

PR for #75 as part of GCI.

Added draw_solid_rect_with_border to AGG renderer so that calling drawSolidRectWithBorderdoes not require overlaying two rectangles made with draw_rectand draw_solid_rect.

KarthikRIyer commented 4 years ago

@boronhub are you on linux or mac?

boronhub commented 4 years ago

linux

boronhub commented 4 years ago

@KarthikRIyer how can i fix the build errors?

WilliamHYZhang commented 4 years ago

@boronhub looking at the error: ld: symbol(s) not found for architecture x86_64, the build is probably experiencing an error compiling some code. I don't know about the specifics, though. is swift build and swift test passing on your local system?

EDIT: probably something to do with the recent merge to update the AGG version... see #93, tests are failing after that push.

boronhub commented 4 years ago

Yeah, it's passing.

boronhub commented 4 years ago

@KarthikRIyer @karwa the tests are failing because of an XCTAssertTrue failed - 🔥 Image mismatch, does that have to do with the recent AGG update ? Even one of my other PRs #96 is failing, even though it is just the addition of one line to the README.

karwa commented 4 years ago

No, the tests are failing here because the output has changed. The thickness of the legend border seems different.

image

boronhub commented 4 years ago

@karwa what's triggering this?

karwa commented 4 years ago

hard to say - you'll have to debug it. You might try doing the stroke after the fill, though.

WilliamHYZhang commented 4 years ago

@boronhub once @karwa's PR gets merged try updating your fork against upstream after you fix the local errors.

karwa commented 4 years ago

That PR won't fix the issue - it only addresses a bug where some tests were not being run on Linux. These are different tests that are failing, and they are failing for good reason: the output has demonstrably regressed.

The legend's default fill colour is not transparent - it is actually white with 70% alpha. If you draw the border first, then fill the same rectangle on top of it, you're going to be drawing over the inner half of the border. Instead, you should fill the rectangle and draw the border on top of it.

boronhub commented 4 years ago

@karwa I made that change, but the tests are still failing.

boronhub commented 4 years ago

@karwa ping

KarthikRIyer commented 4 years ago

@boronhub sorry for the delay. I've been occupied elsewhere and will probably be occupied this weekend. I've extended your deadline by 2 days just in case I'm unable to get to this.

KarthikRIyer commented 4 years ago

I just pulled your changes on my system and ran swift test. All tests seem to pass except the line chart crossBothAxes and it seems to be because the X range of the plot drawn is 0 to 5 and the reference is -5 to 5.

boronhub commented 4 years ago

Thanks @KarthikRIyer! How do I fix the inconsistencies?

boronhub commented 4 years ago

@KarthikRIyer @karwa merging the latest commits made the tests pass !

KarthikRIyer commented 4 years ago

Look good @boronhub. Thanks for your work and patience. Appreciated!