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

VideoCommon: store the configuration used to create the AbstractPipeline #11498

Merged
merged 1 commit into from Feb 9, 2023

Conversation

iwubcode
Copy link
Contributor

This allows us to re-create the pipeline easily (with tweaked changes). used in #11300

@iwubcode iwubcode marked this pull request as draft January 30, 2023 08:01
@iwubcode

This comment was marked as outdated.

@iwubcode iwubcode marked this pull request as ready for review January 31, 2023 06:47
@phire
Copy link
Member

phire commented Jan 31, 2023

It feels slightly sub-optimal to set the AbstractPipeline's config after it's created.

  1. It creates a potential foot-gun where a user of the CreatePipeline function forgets to set the config (and I kind of want to expose Abstract stuff via the scripting interface on my scripting branch)
  2. The memory needs to be written twice. First clearing to zero during initialisation and then copying the data into it a second time.

@iwubcode
Copy link
Contributor Author

iwubcode commented Feb 1, 2023

Thanks @phire for looking it over. I considered doing this in CreatePipeline() originally, can't remember why I didn't go with that. I will take another look.

@phire
Copy link
Member

phire commented Feb 1, 2023

can't remember why I didn't go with that. I will take another look.

Probably because it would have involved making the same change in 6 different files.

@iwubcode
Copy link
Contributor Author

iwubcode commented Feb 1, 2023

Ah yeah. Then I considered doing an "impl" pattern but chickened out. Guess I'll wait on your KillRenderer before making those changes.

@iwubcode iwubcode marked this pull request as draft February 1, 2023 05:00
…ine on the pipeline itself, so that it's easy to duplicate pipelines with slightly altered configuration
@iwubcode iwubcode marked this pull request as ready for review February 9, 2023 08:23
@JMC47 JMC47 merged commit a88e5ef into dolphin-emu:master Feb 9, 2023
14 checks passed
@iwubcode iwubcode deleted the save_pipeline_config branch February 9, 2023 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants