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

Mapbox expressions #2

Closed

Conversation

mvarendorff2
Copy link
Contributor

@mvarendorff2 mvarendorff2 commented Sep 6, 2021

Right, this is a minor chaos and still work in progress.

This PR aims to provide support for a variety of expressions (like case, match, step, interpolate and coalesce) in the Mapbox specification (in an effort to close #1)

The process of getting here was taking the theme we got from mapbox and letting the ThemeReader do its best to parse it without running into errors. To allow for different expressions, two new kinds of classes were added; Parser and Expression with the parsers producing expressions from the theme which can then later be evaluated with information from the render context (zoom level, rotation and a feature's properties).
Existing typedefs for functions were also replaced with Expressions in an effort to unify the types of information that are passed around.
A central parse<T> function may be used to try getting an expression from part of the theme; the type parameter is required to parse for certain types (parse<String> runs different code than parse<double> for example) and the whole system should be reasonably extensible.

@greensopinion
Copy link
Owner

@mvarendorff thanks for the contribution, this looks like some great work! great to see some tests in there too.

I can see that this is a draft, but perhaps I can give some early feedback on things that I'll be looking for:

  • a single pull request with many commits (e.g. more than one feature) makes it a lot harder on the reviewer since there are so many changes, and it's hard to see what is related - could you split this up into a few smaller pull requests? For example, consider splitting rotation from expressions.
  • I'd prefer not to have TODO comments in the code, since they accumulate over time. Either the work is important enough to do, or it's not.
  • no commented code, if it doesn't belong, just delete it instead
  • please leave out prefixes on commit messages (e.g. "WIP", "feat") and instead explain in the message what the commit is/does
  • I'd prefer to see named classes used instead of tuple, since named classes can make code easier to read and it will help to keep the number of dependencies to a minimum

Great to see these improvements coming together!

@mvarendorff2
Copy link
Contributor Author

Thanks for the quick feedback already! I will try to move the text rotation out; it won't be of much use on its own because it also depends on the expressions introduced in the rest of the PR but should indeed be easier to review then!

The rest of the commits I planned to either squash into one or commit batches of files separately eventually because pretty much everything (be it file location, or content) can change and the single commits mostly represent end of work days or me trying to clean up a bit before jumping into the next feature to receive expressions. I would update commit messages accordingly when doing that!

The rest of the feedback I will incorporate into the "final" state of the PR!

@greensopinion
Copy link
Owner

greensopinion commented Sep 7, 2021 via email

@mvarendorff2 mvarendorff2 changed the title WIP: Mapbox expressions and keeping text upright Mapbox expressions Sep 21, 2021
@mvarendorff2 mvarendorff2 marked this pull request as ready for review September 21, 2021 14:57
@mvarendorff2
Copy link
Contributor Author

I finally found some time to work on this again and clean things up a bit.

I moved keeping the text upright to mvarendorff#1 for now so you can have a look at the isolated diff if you want. It does depend on this PR though so it merges into the branch this PR is made from.

Let me know what you think!

@greensopinion
Copy link
Owner

greensopinion commented Sep 24, 2021 via email

@mvarendorff2
Copy link
Contributor Author

Sounds good! Just to make sure this doesn't end up stale after a misunderstanding: This PR would be ready to check as well :)

Copy link
Owner

@greensopinion greensopinion left a comment

Choose a reason for hiding this comment

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

I'll have a more thorough look later, but have a question regarding dependencies.

Copy link
Owner

@greensopinion greensopinion left a comment

Choose a reason for hiding this comment

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

This PR is 1286 lines of code, much of it new - so it's very hard to do a proper code review.

Could you split this into a few smaller PRs, ideally 3-400 lines of code each? For example, you could start with a PR to introduce the concept of expressions for existing functionality before introducing various new parsers and expressions. That makes the implications of various changes much easier to reason about. It will also reduce back-and-forth since with a smaller initial PR, you'll be able to avoid rework on subsequent changes.

Overall this looks quite good. I've added some comments and questions below.

lib/src/parsers/parser.dart Outdated Show resolved Hide resolved
lib/src/parsers/parser.dart Outdated Show resolved Hide resolved
lib/src/parsers/parser.dart Show resolved Hide resolved
lib/src/parsers/case.dart Show resolved Hide resolved
lib/src/parsers/boolean.dart Outdated Show resolved Hide resolved
lib/src/parsers/parsers.dart Outdated Show resolved Hide resolved
lib/src/parsers/step.dart Show resolved Hide resolved
lib/src/expressions/expression.dart Outdated Show resolved Hide resolved
import 'theme_function_model.dart';

abstract class ThemeFunction<T> {
Map<FunctionModel, _ZoomValue> _cache = {};

T? exponential(FunctionModel<T> model, double zoom) {
T? exponential(FunctionModel<T> model, Map<String, dynamic> args) {
final zoom = args['zoom'] as double;
Copy link
Owner

Choose a reason for hiding this comment

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

now that we're passing args and not just a zoom, the cached value could be incorrect depending on the arguments used by the function, right? in other words, is this cache now broken?

The idea behind the cache is to optimize rendering, i.e. only calculate expensive expressions once when rendering. I found that this made a big difference in the profiler, since tiles can result in evaluating expressions 1000s of times, and some expressions are expensive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point and honestly, I am not sure. The arguments passed to the function are just the properties of the VectorTileFeature that's currently being rendered which by my understanding stay identical per feature regardless of the zoom making the zoom level the only relevant property from the args for caching. I am just uncertain whether my assumption of the properties of VectorTileFeatures remaining identical is accurate or not (if it was accurate, maybe it's still enough because the properties are still tied to the zoom level meaning the zoom level would still be sufficient as cache key).

What's your take on this?

Copy link
Owner

Choose a reason for hiding this comment

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

If the properties of the VectorTileFeature are used in calculations, then this needs to be reworked since the cache would be broken. If they're not used in calculations, then they should not be passed in because it could lead to future bugs and makes the code more complicated.

@greensopinion
Copy link
Owner

greensopinion commented Oct 5, 2021 via email

@mvarendorff2
Copy link
Contributor Author

Don't mind the force push; following your request I will split off a smaller piece of this thing and force push that as sole commit to this PR, then once that is merged, take the next chunk and so on.

@greensopinion
Copy link
Owner

Great, thanks

@greensopinion
Copy link
Owner

@mvarendorff I've done some work on expressions over the past couple of days that has duplicated some of the functionality on this pull request. There's still more to be done but it's a start and addresses the issue around caching. You'll notice that I've skipped generics, which I think helps to simplify the implementation.

I'd be interested to know what you think, whether there are any major shortcomings or missing functionality in what I've done.

this library now depends on bahung1221/dart-vector-tile#6 which is not yet merged

@mvarendorff2
Copy link
Contributor Author

Hey there! Thanks for the time you put into that matter and sorry for the hiatus. Sadly the time I had ran out much earlier than I thought and I didn't have a chance to work on this again further than to throw it in our project and hope for it to work :D

Your approach looks a tad more extensible with the way you register parsers; I am personally a fan of generics because it avoids a bunch of (usually implicit) dynamic values to fly around with everyone hoping they are the right type but noone really knows for sure unless they went through the entire code. TypeScript made me fear any, and dynamic is too close for comfort ;)

I am also not sure whether it's a good idea to keep all parsers in one file; it's already 400+ lines long and there are several expressions still missing (case, coalesce and step for example).

Last thing I noticed was the missing Color support in the current expression implementation. Especially in interpolations which are a common usecase for that (fading opacity based on zoom level for example) the expression just returns null if the base is not a number. That was also one of the reasons I kept the generics in there because they allowed for passing an expected type from the place of parsing (e.g. line-color has to be parsed as a color, opacities always have to be double, etc) which is not possible now from what I can tell but required since it can be too late at evaluation time (since you cannot interpolate a string directly but have to make sure it's a Color / double at that time already and know which one it is). Alternatively evaluate would have to take a generics parameter and evaluate would have to contain the respective logic to make sure the values are the correct type.

Out of curiosity because I couldn't find the respective section in the code: How did you address caching?

@greensopinion
Copy link
Owner

Hey there! Thanks for the time you put into that matter and sorry for the hiatus. Sadly the time I had ran out much earlier than I thought and I didn't have a chance to work on this again further than to throw it in our project and hope for it to work :D

You're welcome, and not to worry!

I am also not sure whether it's a good idea to keep all parsers in one file; it's already 400+ lines long and there are several expressions still missing (case, coalesce and step for example).

Agreed, we can split these out if/when the file gets too big (maybe it is already)

Last thing I noticed was the missing Color support in the current expression implementation.

:D I didn't get to everything. I agree that Color is missing, feel free to contribute if you want. I couldn't take it all on at once and didn't have a need just yet.

As for type safety, that's an improvement that we could add too if it doesn't clutter the code up too much. I agree with your misgivings about everything dynamic, but also want the code to stay clean and maintainable.

Out of curiosity because I couldn't find the respective section in the code: How did you address caching?

It's a worse-is-better approach: interpolate_expression.dart#L121
It's not pretty, but seems to work well enough.

@greensopinion
Copy link
Owner

Closing for now, since some of this is done. Feel free to open an issue if you want to discuss further, or a new pull request if you want to continue improving it.

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.

Support interpolate, step and case expressions for color and number values
2 participants