Skip to content

Event API#26

Closed
pka wants to merge 22 commits into
mainfrom
event-api
Closed

Event API#26
pka wants to merge 22 commits into
mainfrom
event-api

Conversation

@pka
Copy link
Copy Markdown
Member

@pka pka commented May 8, 2022

This PR adds an event API, which should eventually replace the direct method call API. Main advandages are:

  • Support for chained processors (included are PromoteToMulti and LonLatToMercator)
  • GeomVisitor struct tracks main geometry type and collection flag, which can be used in format writers
  • Optional validation of event sequence
  • Easier serialization for sending events to other processes

Next steps:

  • Review API, escpecially exported modules
  • Compare performance with direct method call API (FlatGeoBuf writer)
  • Review FeatureProcessor API. Should it be also event based?

@michaelkirk
Copy link
Copy Markdown
Member

I'm hoping this is related to #22 (comment)

I'm eager to see how it shakes out - let me know if you'd like me to test something.

@pka pka force-pushed the event-api branch 3 times, most recently from 28d6ea3 to c3c4dec Compare May 11, 2022 22:34
@pka
Copy link
Copy Markdown
Member Author

pka commented May 14, 2022

It's slowly getting serious! Chaining like in #22 now supported.

@pka pka force-pushed the event-api branch 3 times, most recently from 8dab8c5 to 8209707 Compare May 15, 2022 20:52
Copy link
Copy Markdown
Member

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

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

It's slowly getting serious! Chaining like in #22 now supported.

Nice! I've just taken a quick look at it, and it seems promising!

Do you think it's ready to attempt something like #36 (comment)? I can give it a try if you think so.

Comment thread geozero/src/chaining.rs Outdated
// ------- Chain events -------

/// Processing geometry events and passing events to a chained visitor
pub trait CĥainedGeomEventProcessor {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh wow, programming with diacritics! Exciting! 😉

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No idea, how I did that 🤣

@pka
Copy link
Copy Markdown
Member Author

pka commented May 16, 2022

It's slowly getting serious! Chaining like in #22 now supported.

Nice! I've just taken a quick look at it, and it seems promising!

Do you think it's ready to attempt something like #36 (comment)? I can give it a try if you think so.

Will have to support the event API in a format writer first. What's your favorite output format you want apply reprojection? geo-types?

@michaelkirk
Copy link
Copy Markdown
Member

michaelkirk commented May 16, 2022

Will have to support the event API in a format writer first. What's your favorite output format you want apply reprojection? geo-types?

geo-types would be great!

@pka pka marked this pull request as ready for review May 22, 2022 17:23
@pka
Copy link
Copy Markdown
Member Author

pka commented May 29, 2022

Testing the event API by implementing it for the FlatGeoBuf writer resulted in some major changes (e.g. bd3f1dd) and the following open points:

  • processor type in GeomVisitor shouldn't be defined at compile time to support user flags like promote-to-multi yes/no
  • Performance of a combination of 3 processors (promote-to-multi, bbox and FGB feature writer) is currently 60% slower than the previous implementation (372us vs. 229us for countries.fgb)

Ideas to investigate:

  • New event type with coordinates slice instead of single coordinates
  • Compare with event API implementation as a single processor
  • Reduce size of event enum
  • Combine chained processors into single FSM

@pka pka marked this pull request as draft July 21, 2022 20:18
@pka
Copy link
Copy Markdown
Member Author

pka commented Aug 12, 2023

Experimental branch is kept in https://github.com/pka/geozero/tree/event-api

@pka pka closed this Aug 12, 2023
@pka pka deleted the event-api branch August 12, 2023 21:55
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.

2 participants