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

feat(examples): Port bezier example shaders to GLSL 300 ES #8433

Merged
merged 2 commits into from Mar 8, 2024

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Jan 18, 2024

For #7457. Ports remaining GLSL (that I could find in code search, anyway) to GLSL 300. I see some errors when trying to start these examples — should they be working? — but I think that has to do with the examples relying on older deck.gl releases or build configuration.

Changed:

  • examples/experimental/bezier (deck ^8.8.0, tested)
  • showcases/ascii/ascii-layer/ (deck ^6.0.0, unable to test)
  • showcases/wind (deck ^5.1.0, unable to test)

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Jan 18, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @donmccurdy and the rest of your teammates on Graphite Graphite

@felixpalmer
Copy link
Collaborator

I came across these as well, but hesitate to change them if they are not working. They are old examples, which as you say were written against an old version of deck. As they are not present on the website, I don't see them as high priority to fix.

Changing the shaders to GLSL 300 without making them work in v9 will mean they are broken in all versions of deck.gl, so perhaps a better option for now is to add a comment stating what version they work against and if someone wants to port them in the future, they can

Copy link
Collaborator

@felixpalmer felixpalmer left a comment

Choose a reason for hiding this comment

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

For the examples which do not work, could you verify which version of deck these were written against (or work with) and add a comment rather than porting the shaders?

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Jan 19, 2024

Thanks, I've added versions to the description above — the working example uses v8, the broken examples use v6 and v5.

I'm a bit concerned about having chunks of dead code: it is more difficult to know when migration tasks are finished as a result. I'm not sure that comments can help with that problem much. If these examples are available on their respective release branches, and haven't been updated since the v7 release (5 years ago) then I am inclined to suggest they shouldn't be on the master branch going into v9.

@felixpalmer
Copy link
Collaborator

I'm a bit concerned about having chunks of dead code

I agree, v5 & 6 are very old. How about you remove the broken examples from this PR and make another PR which just removes them from the codebase, so we give the others a chance to comment?

@donmccurdy donmccurdy mentioned this pull request Feb 14, 2024
7 tasks
@donmccurdy donmccurdy changed the title feat(examples): Port remaining shaders to GLSL 300 ES feat(examples): Port bezier example shaders to GLSL 300 ES Mar 7, 2024
@donmccurdy
Copy link
Collaborator Author

I've removed the changes to the "ascii" and "wind" showcases from this PR, so now it only affects examples/experimental/bezier, which I've tested. I posted a thread in Slack about removing the showcases/ folder, but haven't gotten replies on that, so inclined to just leave those alone for now.

@coveralls
Copy link

coveralls commented Mar 7, 2024

Coverage Status

coverage: 81.577%. remained the same
when pulling 6f54555 on donmccurdy/glsl300-v9-pt2
into f362eb9 on master.

@donmccurdy donmccurdy merged commit 9f1bc3f into master Mar 8, 2024
4 checks passed
@donmccurdy donmccurdy deleted the donmccurdy/glsl300-v9-pt2 branch March 8, 2024 14:38
@donmccurdy donmccurdy added this to the v9.0 milestone Mar 8, 2024
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

3 participants