Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Properties refactor #6868

Merged
merged 1 commit into from
Nov 18, 2016
Merged

[core] Properties refactor #6868

merged 1 commit into from
Nov 18, 2016

Conversation

jfirebaugh
Copy link
Contributor

@jfirebaugh jfirebaugh commented Nov 1, 2016 •
edited
Loading

First item from #4860.

The strategy here is outlined in https://github.com/mapbox/cpp/blob/master/C%2B%2B%20Structural%20Metaprogramming.md.

The interesting parts to look at are paint_property.hpp and layout_property.hpp and one of the generated layer properties, like circle_layer_properties.hpp.

@mention-bot
Copy link

@jfirebaugh, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yhahn, @brunoabinader and @kkaefer to be potential reviewers.

kkaefer
kkaefer reviewed Nov 1, 2016
View reviewed changes
src/mbgl/style/layers/symbol_layer_properties.hpp
};

struct SymbolSpacing : LayoutProperty<float> {
static float defaultValue() { return 250; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using static functions instead of a constexpr member var?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some property types are not constexpr (std::string, std:array<float, 2>). It's possible we could use constexpr type variants (const char *, float[2]) for defaultValue but for now I just went with this simple solution. It'll probably be inlined anyway.

@jfirebaugh jfirebaugh force-pushed the shaders-refactor branch 4 times, most recently from 74b724e to 0782253 Compare November 2, 2016 17:37
@jfirebaugh jfirebaugh mentioned this pull request Nov 4, 2016
Merged
1 task
@jfirebaugh jfirebaugh force-pushed the shaders-refactor branch 2 times, most recently from b7f71e7 to f41e21d Compare November 4, 2016 21:45
@jfirebaugh jfirebaugh force-pushed the shaders-refactor branch 2 times, most recently from aa791f9 to 020260d Compare November 7, 2016 20:27
@jfirebaugh jfirebaugh changed the base branch from shaders-refactor to master November 8, 2016 16:55
@jfirebaugh jfirebaugh force-pushed the properties-refactor branch 2 times, most recently from 709232d to 1a79317 Compare November 11, 2016 16:43
@jfirebaugh
Copy link
Contributor Author

This is ready for review. Like #6856, it takes a fair amount of work to keep up to date against master, so I'd like to merge it soon.

@jfirebaugh jfirebaugh force-pushed the properties-refactor branch 2 times, most recently from 8cee799 to 0e30f2d Compare November 14, 2016 22:08
kkaefer
kkaefer approved these changes Nov 15, 2016
View reviewed changes
include/mbgl/style/transition_options.hpp
duration ? duration : defaults.duration,
delay ? delay : defaults.delay
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This function produces a weird call pattern: the method is called defaults which makes me think it returns defaults, but actually, it returns the values of the current objects if they are set, otherwise the defaults that are passed into this method. Can we come up with a clearer way of using the default values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was modeling on _.defaults but would be fine with renaming it to reverseMerge or something like that.

src/mbgl/style/paint_property.hpp
};

} // namespace style
} // namespace mbgl
} // namespace mgbl
Copy link
Contributor

Choose a reason for hiding this comment

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

No.

src/mbgl/style/layout_property.hpp
};

} // namespace style
} // namespace mbgl
} // namespace mgbl
Copy link
Contributor

Choose a reason for hiding this comment

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

No.

@jfirebaugh
[core] Convert style properties to a tuple-based approach
38fcbe2 
This converts the style property classes (CirclePaintProperties and so on) to the same tuple-based approach as gl::Attribute and gl::Uniform. The approach is outlined in https://github.com/mapbox/cpp/blob/master/C%2B%2B%20Structural%20Metaprogramming.md.

The main advantage of this approach is it allows writing algorithms that work on sets of style properties, without resorting to code generation or manually repetitive code. This lets us iterate on approaches to data-driven properties more easily.

Another advantage is that the cascading, unevaluated, and evaluated states of a set of properties exist as independent structures, instead of individual properties holding their own state. This is a more functional approach that makes data flow clearer and reduces state.
@jfirebaugh jfirebaugh merged commit 38fcbe2 into master Nov 18, 2016
@jfirebaugh jfirebaugh deleted the properties-refactor branch November 18, 2016 01:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kkaefer kkaefer kkaefer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jfirebaugh @mention-bot @kkaefer @1ec5