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

celiagg Kiva backend #247

Merged
merged 9 commits into from
Dec 23, 2016
Merged

celiagg Kiva backend #247

merged 9 commits into from
Dec 23, 2016

Conversation

jwiggins
Copy link
Member

@jwiggins jwiggins commented Oct 26, 2016

This requires https://github.com/celiagg/celiagg (or pip install celiagg!) to be built and installed.

This works fairly well now. We should discuss how to proceed.

@codecov-io
Copy link

codecov-io commented Oct 26, 2016

Current coverage is 52.05% (diff: 100%)

Merging #247 into master will not change coverage

@@             master       #247   diff @@
==========================================
  Files           126        126          
  Lines         13176      13176          
  Methods           0          0          
  Messages          0          0          
  Branches       1943       1943          
==========================================
  Hits           6859       6859          
  Misses         5872       5872          
  Partials        445        445          

Powered by Codecov. Last update 3e1594b...4d6ebb3

'text_transform', 'font'])


class GraphicsContext(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

It might make sense to inherit from AbstractGraphicsContext or basecore2d.GraphicsContextBase. Although at this point it may be more hassle than its worth.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see that you've registered it with the ABC below. Ignore the above, then.

@corranwebster
Copy link
Contributor

Yay! Will try this out at some point soon.

raise NotImplementedError(msg)

def set_text_drawing_mode(self):
""" XXX: What is this for??
Copy link
Contributor

Choose a reason for hiding this comment

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

This is how to draw text: filled, outline, etc. See constants.py for the options.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahhh... You can probably see that I based this on the QPainter backend. Also, based on what I've read in the AGG backend, this isn't possible to implement there (fonts are raster only).

Do you know of a backend which implements this correctly? Quartz?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, Quartz, although it is cheating somewhat because it just maps to the appropriate Quartz constant and passes it through to Quartz's corresponding method.

The raster-only stuff is likely why the current agg backend has a vendorized freetype2. Eg. see https://github.com/enthought/enable/blob/master/kiva/agg/src/kiva_graphics_context_base.cpp#L579, https://github.com/enthought/enable/blob/master/kiva/agg/src/kiva_graphics_context_base.cpp#L644 and https://github.com/enthought/enable/blob/master/kiva/agg/src/kiva_graphics_context.h#L1286

Copy link
Contributor

Choose a reason for hiding this comment

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

There are ctypes-based FreeType wrappers for Python (eg. https://pypi.python.org/pypi/freetype-py/) but I don't think they will help. I didn't see a Cython-based wrapper, which would be nicer for this use case.

Copy link
Member Author

Choose a reason for hiding this comment

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

pyagg already has support for vector and raster font representations. In fact, I've got it hard-coded to use vectors in the current backend code (see agg.FontCacheType.VectorFontCache in the set_font method). That should basically cover it, right (minus the complication of switching between representations in the Font instances)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, if pyagg has vector font support that should cover it if you can specify the render method (fill, stroke, etc.) for the vector path of the text.

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented!

@jwiggins
Copy link
Member Author

I removed some previously committed changes to the examples... Force pushed. Mind the gap.

This is almost usable now. 😄

@jwiggins
Copy link
Member Author

This is now functionally complete. Bugs included at no extra cost to you!

Also, fixed the implementation of CompiledPath.add_path
@jwiggins
Copy link
Member Author

I've fixed all the bugs I could find. This actually works pretty well with all of the Chaco demos now!

Still faking it, but just about making it!
@jwiggins jwiggins changed the title [WIP]: pyagg Kiva backend pyagg Kiva backend Oct 30, 2016
@jwiggins jwiggins changed the title pyagg Kiva backend celiagg Kiva backend Nov 9, 2016
@corranwebster corranwebster added this to the 5.0.0 Release milestone Dec 19, 2016
@corranwebster
Copy link
Contributor

corranwebster commented Dec 22, 2016

Playing around with this right now. I noticed that there are some artifacts when using gc.draw_image and not aligned to exact pixel edges. Eg.

gc.draw_image(a, (200, 400, 100, 100))

looks like this:
screen shot 2016-12-22 at 2 29 13 pm
(notice line at right border) where if I force image to draw precisely on pixel boundaries, eg.

gc.draw_image(a, (200.5, 400.5, 100, 100))

there is no such problem:
screen shot 2016-12-22 at 2 32 15 pm

Possibly related to the alpha?

Image is generated by:

a = np.empty((100, 100, 4), dtype='uint8')
a[:, :, 0] = np.linspace(0, 255, 100)
a[:, :, 1] = np.linspace(0, 255, 100).reshape(-1, 1)
a[:, :, 2] = np.linspace(255, 0, 100)
a[:, :, 3] = np.linspace(255, 0, 100).reshape(-1, 1)

@corranwebster
Copy link
Contributor

Not sure if I should be putting these into the celiagg repo or here?

There is an issue that coloring isn't working for fonts. In the default kiva_explorer.py script, the fonts should be coloured like this:
screen shot 2016-12-22 at 2 42 49 pm
but instead are just rendererd in black
screen shot 2016-12-22 at 2 42 18 pm
(although FWIW the text layout is better)

@corranwebster
Copy link
Contributor

corranwebster commented Dec 22, 2016

To answer the question about how to proceed. There are two ways we could do this:

  • the way that this is structured now, where we have the current Agg backend and the new Celiagg backend and you access them with ETS_TOOLKIT=....celiagg or whatever. We would need to be careful about use of the current Image graphics context and code shouldn't assume the existence of Agg or Celiagg for rendering images (maybe lean more heavily on Pillow for that stuff).
  • we replace the Agg backend with the Celiagg backend everywhere. We'd need Celiagg to be reasonably feature-comparable to the current backend before doing this.

I'm not sure which is best. I'd be happy to see the back of the current Agg wrappers, but they work for the most part, so it may make sense to have a more gradual transition

@jwiggins
Copy link
Member Author

The font color issue is an interesting one. If you try kiva_explorer.py with the Quartz backend, you will see that it matches celiagg. Basically, some backends use stroke color for text, others use fill color. I kinda punted and followed Quartz, but honestly it's not hard to change.

@corranwebster
Copy link
Contributor

Hmmm... The qpainter backend also uses stroke color for text. Given that changing those is likely to be difficult (QPainter is just reflecting the underlying behaviour of Qt, I imagine Quartz is similar) I'm willing to live with using set_stroke_color instead of set_fill_color for celiagg.

Making sure we are consistent in the change throughout the dependent codebases will be a little finicky.

@corranwebster
Copy link
Contributor

Thought a bit more about the way forward. I'm happy for this to be merged as-is so that we can work on further improvements based on this. We can revisit what to do with the current agg backend as things go forward.

@jwiggins
Copy link
Member Author

I'd like to explore the image drawing issue a little bit first, but I suspect that it lies completely within celiagg. We shall see.

After that, yes, I think this should just be merged since it's not disruptive on its own. Then we can begin to discuss the more disruptive changes.

@jwiggins
Copy link
Member Author

Okie dokie. I'm pulling the trigger. Thanks for the feedback Corran!

@jwiggins jwiggins merged commit 04d82e0 into master Dec 23, 2016
@jwiggins jwiggins deleted the feat/pyagg-backend branch December 23, 2016 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants