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

preprocessor directives followup #89

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

chrishavlin
Copy link
Contributor

This PR follows on #70 and

  • adds the ability to toggle preprocessor directives interactively (by triggering re-compilation)
  • replaces the depth buffer toggle (use_db) with a preprocessor USE_DB definition. I'm actually not sure switching this to a pre-processor directive is better or worse, but it does nicely illustrate the functionality.

@chrishavlin chrishavlin added the enhancement New feature or request label Jun 21, 2023
@chrishavlin chrishavlin marked this pull request as draft June 21, 2023 22:27
@chrishavlin
Copy link
Contributor Author

draft for now (but feel free to look now, @matthewturk ). I went through a couple iterations of approaches, want to double check that I didn't leave behind unwanted code... but also wanted to push this up now :)

@chrishavlin
Copy link
Contributor Author

oh, also a draft cause I didn't actually run the tests locally. oops. so will have to fix a few things. but right now: running examples/amr_volume_rendering.py and toggling the depth buffer box DOES work. just need to better handle a few things to get those tests passing...

self.colormap_fragment = new_combo["second_fragment"]
self.vertex_shader = new_combo[
"first_vertex"
], self._program1_pp_defs.get_shader_defs("vertex")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tests are failing cause self._program1_pp_defs (and 2) can be None.

Copy link
Member

Choose a reason for hiding this comment

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

Should we instead have them not allow none, and supply a default that is an empty preprocessor state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya, I do think that'd be better!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants