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

Rendering modularization proposal. #547

Merged
merged 22 commits into from Dec 14, 2022
Merged

Conversation

sjg-wdw
Copy link
Collaborator

@sjg-wdw sjg-wdw commented Oct 28, 2022

Hello MapLibre Community!

This is our first Pull Request of two for the Metal upgrade planning effort. This one focuses on modularizing the renderer in preparation for supporting multiple rendering SDKs at the same time (or at least the same code base).

The PR is designed to be commented on. We've put our Goals in and we'd like to hear your Goals for the Renderer Modularization. Just that for now. The second PR will focus on Metal for iOS.

As we work on the details of the implementation, we will expand the Proposed Change section. At the same time we will discuss any new Goals that turn up and prioritize them based on community feedback.

In 3-4 weeks we will be done with our Proposed Changes and will hopefully have addressed all the new Goals. The PR can then be merged if agreed.

@lseelenbinder lseelenbinder marked this pull request as draft October 28, 2022 07:37
@lseelenbinder
Copy link
Member

Thanks @sjg-wdw! Looking forward to the discussion. I did convert this to a draft so we don't accidentally merge it early. 😄

Copy link
Member

@lseelenbinder lseelenbinder left a comment

Choose a reason for hiding this comment

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

On a high-level, I think this very much aligns with my goals and hopes for the project. Thank you for preparing this!

One goal I saw that I would phrase differently. I, personally, don't think pixel-for-pixel rendering is necessarily the best way to state goal. I think it should be visually correct and within a visually equivalent delta (1%? 2%?) of the current solution. It's very likely a large overhaul like this will expose bugs and errors in the present code, which should not be preserved for the extrinsic goal of pixel-for-pixel reproducibility.

Other than this, I think this is a great starting point, and I look forward to the continued additions.

@sjg-wdw
Copy link
Collaborator Author

sjg-wdw commented Oct 28, 2022

Thanks for moving this to draft status and bearing with us in this process.

For the initial modularization we probably could match the old output perfectly. We're just going to be moving things around, using the same graphics SDK, and so forth. But if we do end up fixing something, perhaps it would change, that's a good point.

Certainly for the next PR (Metal) the pixels will change. Metal just works differently. So it might be good to nail down what's a good goal.

@lseelenbinder
Copy link
Member

For the initial modularization we probably could match the old output perfectly. We're just going to be moving things around, using the same graphics SDK, and so forth. But if we do end up fixing something, perhaps it would change, that's a good point.

Certainly for the next PR (Metal) the pixels will change. Metal just works differently. So it might be good to nail down what's a good goal.

Ah, yes, of course. I conflated the two steps. Let's clarify it as what you just said:

  • For the modularization, pixel-for-pixel, bug fixes excluded.
  • For the Metal transition, find a way to say: "preserve the spirit of the rendering without having to match pixel-for-pixel."

@wipfli
Copy link
Member

wipfli commented Nov 3, 2022

Looks nice, thanks for putting this together, Steve. A few goals people have are also documented here: #540 I don't know how it fits into this propsal but just wanted to share...

@thehoneymad
Copy link
Collaborator

@sjg-wdw, thank you for putting this together. Following the chat we had, waiting with excitement with specific changes to be proposed.

@sjg-wdw
Copy link
Collaborator Author

sjg-wdw commented Nov 4, 2022

Something I'm hearing from a number of organizations is issues around library size.
Should that be an explicit goal of this effort?

Pro: We're probably going to make the library slightly bigger with the modularization. We should make it slightly smaller in other areas to compensate. Otherwise no existing app developers are likely to adopt it.
Con: If it's unrelated to renderer modularization, does it belong here?

@thehoneymad
Copy link
Collaborator

Could we not do selective compilation to reduce the binary size even with modularization? I do not think it is a hard blocker for our improvement proposal. :)

@sjg-wdw
Copy link
Collaborator Author

sjg-wdw commented Nov 4, 2022

Not a blocker, no. I'm just thinking we could try to improve adoption of the post-modular, but pre-Metal version if it didn't get any bigger.

One thing I've noticed with Maplibre Native is that companies are generally supportive but haven't explicitly switched to it because it does nothing new they need yet. Metal is obviously an answer to that problem, but 'got smaller' might also be.

@mwilsnd
Copy link
Collaborator

mwilsnd commented Nov 9, 2022

Do we have a plan to address shader permutations as we decouple from maplibre-js (data driven by #pragma mapbox via either uniforms or vertex attributes)? Needing to support permutations likely locks us out of any precompiled shader support in future backends like Metal.

I've also observed with Metal specifically, shipping a compressed text version of shaders can be more space efficient so perhaps that should be the approach we focus on.

@sjg-wdw
Copy link
Collaborator Author

sjg-wdw commented Nov 9, 2022

For OpenGL the Shader Registry will just hand you back a shader based on a well named request. So the internal logic can do any numbers of things. I wasn't going to specifically address pragmas in there in this effort, just wrap up what was already there.

I am implicitly backing away from shader construction, though. The current code does that by slapping bits together in a fairly simple way. Obviously a wrapper around that logic will continue to do it. But for new implementations I would hope we move to shaders that contain everything they need. They're much easier to debug.

For Metal it's a shame to ship shader source. You end up adding a compile step which takes a bit of time. If that's required, then we can figure it out. Perhaps have a Metal variant of the Shader Registry that does that. I don't really love it, though. It's going to be a weird debugging experience.

@sjg-wdw
Copy link
Collaborator Author

sjg-wdw commented Nov 18, 2022

We've finished the PR, adding several new sections and addressing feedback.
Feel free to reach out for more detail if you need it.

@wipfli
Copy link
Member

wipfli commented Nov 21, 2022

If this is ready for review, you can remove the draft status @sjg-wdw

@sjg-wdw sjg-wdw marked this pull request as ready for review November 21, 2022 15:30
Copy link
Member

@wipfli wipfli left a comment

Choose a reason for hiding this comment

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

Beautiful, you have put together an amazing proposal, @sjg-wdw! Thanks for diving into the Native library and for writing this design proposal. I added a few typo fixes and minor comments.

design-proposals/2022-10-27-rendering-modularization.md Outdated Show resolved Hide resolved
design-proposals/2022-10-27-rendering-modularization.md Outdated Show resolved Hide resolved
design-proposals/2022-10-27-rendering-modularization.md Outdated Show resolved Hide resolved
design-proposals/2022-10-27-rendering-modularization.md Outdated Show resolved Hide resolved
design-proposals/2022-10-27-rendering-modularization.md Outdated Show resolved Hide resolved
design-proposals/2022-10-27-rendering-modularization.md Outdated Show resolved Hide resolved
design-proposals/2022-10-27-rendering-modularization.md Outdated Show resolved Hide resolved
@wipfli
Copy link
Member

wipfli commented Nov 22, 2022

I am a big fan of development targeting the main branch. It just makes it easier to coordinate between different contributions. Do you think we can target your pull requests to main?

@wipfli
Copy link
Member

wipfli commented Nov 22, 2022

Each pull request has a section about the tests you plan to add. Could we also add examples for the new stuff? I really would love to build up more examples in MapLibre Native. I don't know what the best format is, but just some small snippets that show how one can use different APIs would be super helpful...

@sjg-wdw
Copy link
Collaborator Author

sjg-wdw commented Nov 22, 2022

I think examples are a nice idea.

@sjg-wdw
Copy link
Collaborator Author

sjg-wdw commented Nov 22, 2022

I am a big fan of development targeting the main branch. It just makes it easier to coordinate between different contributions. Do you think we can target your pull requests to main?

Seems reasonable. Each PR is meant to be self contained.

sjg-wdw and others added 4 commits November 22, 2022 13:20
Co-authored-by: Oliver Wipfli <oliver.wipfli@leichteralsluft.ch>
Co-authored-by: Oliver Wipfli <oliver.wipfli@leichteralsluft.ch>
Co-authored-by: Oliver Wipfli <oliver.wipfli@leichteralsluft.ch>
Co-authored-by: Oliver Wipfli <oliver.wipfli@leichteralsluft.ch>
@sjg-wdw
Copy link
Collaborator Author

sjg-wdw commented Dec 12, 2022

Following up here on what you're asking for @JannikGM. You are asking for a different approach, specifically using an intermediate layer to interpret low level rendering requests for different toolkits depending on the platform.

There are a number of layers out there already like that, so I'd say that approach would lead to evaluating those instead of writing a new one.

We looked into this early on, discussed it with the community, and came to the conclusion that it would add a lot of size to the toolkit. We know for certain that MetalANGLE does at one extreme. It seems likely that even the lightest toolkit would as well.

I'd also add that when you use one of those you still have a problem with multi-platform support. UI toolkits are similarly popular in the mobile world and big users have come to the conclusion that they're not less work, per se, just different work. They need to have experts in the toolkit and experts in how it interacts with each platform.

However, if you ignore the size issue it's a valid approach. If you feel strongly about it, please articulate your plan.

@sjg-wdw
Copy link
Collaborator Author

sjg-wdw commented Dec 12, 2022

A note for everyone who's commented and/or signed off on this PR. Thanks!
Also, we have the Metal Port PR up now too.

If you'd like to comment, please do!
If you don't want to or don't feel qualified, that's fine too. It's a much more focused on the specifics of Metal.

@sjg-wdw
Copy link
Collaborator Author

sjg-wdw commented Dec 12, 2022

I'm following up on another issue that @JannikGM brought up. The idea of adding shaders to the toolkit. It's fundamental to our redesign here and I think we should hash out that issue in detail.

We support several key issues involving shaders:

  • That they be named and addressable outside the toolkit
  • That they be replaceable by the developer individually without replacing core parts of the toolkit
  • That they be flexible enough to support existing functionality while still allowing developers to add in their own functionality.

I'm not 100% sure we both understand this the same way.

For example, I'm not saying that a developer has to provide their own shaders. Not at all. We'll provide a core set of shaders to do everything the toolkit does currently. In this rework we're just suggesting wrapping up the existing shaders in this way.

If a developer did want to completely replace all the shaders used by the standard toolkit, they could. Not sure why they would, but they could.

So my question is what part of that do you see as a problem?

@lseelenbinder lseelenbinder merged commit ff2745b into maplibre:main Dec 14, 2022
@lseelenbinder
Copy link
Member

Discussed in TSC for final approval and has been merged. Thanks @sjg-wdw and everyone else involved!

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.

None yet