Improved Settings changes round 1#2676
Conversation
|
CI gfxreconstruct build queued with queue ID 645205. |
MarkY-LunarG
requested review from
antonio-lunarg,
bradgrantham-lunarg,
davidd-lunarg,
fabian-lunarg,
ncesario-lunarg and
panos-lunarg
and removed request for
a team
|
CI gfxreconstruct build # 8845 running. |
|
Also introduces a README in |
|
CI gfxreconstruct build # 8845 passed. |
ncesario-lunarg
left a comment
ncesario-lunarg
left a comment
There was a problem hiding this comment.
LGTM! I haven't gone through the python script yet, but will try to get to that sometime tomorrow. The comments I have are largely suggestive and nothing I would block merging over.
There was a problem hiding this comment.
Should this be in framework/generated? I actually think this naming scheme makes more sense (i.e., put generated files--which are already prefixed with "generated_"--in the directories they are associated with), but it looks like there is no precedent for this.
There was a problem hiding this comment.
This is actually something I want more feedback on. These generated source files are built from scripts/settings_options and yes, I agree that the generated files make more sense to me in their relevant directory. If people feel they belong in the generated folder, I will also move the script from scripts/settings into the generated folder. The problem I had is none of these items are "framework" specific and it also touches files at the top level as well.
There was a problem hiding this comment.
I would vote for keeping it as you have it, but I'm curious what others think.
| Python3 and then install the appropriate "jsonschema" package. | ||
|
|
||
| ```bash | ||
| pip install jsonschema |
There was a problem hiding this comment.
May want to use a requirements.txt to track what's necessary to run the script.
There was a problem hiding this comment.
We don't have one right now. Should we?
Right now, I think of 2 items we probably need overall for GFXR and that is:
jsonschema==4.10.3
lxml==5.2.1
The versions might be different, but that's what's supported on my system. What do you think? We should run this by the group, though.
There was a problem hiding this comment.
I would say add the requirements.txt to track this, but yeah, needs more input.
MarkY-LunarG
added
the
approved-to-run-ci
MarkY-LunarG
force-pushed
the
marky-improve-settings-capture
branch
from
3ffbfbb to
716e356
Compare
|
CI gfxreconstruct build queued with queue ID 647850. |
|
CI gfxreconstruct build # 8879 running. |
|
CI gfxreconstruct build # 8879 passed. |
There was a problem hiding this comment.
Is the purpose here to replace the settings from framework/encode/capture_settings.h?
As far as I understand right now it just duplicates the settings
There was a problem hiding this comment.
Yes, this will eventually replace capture settings. However, it's the first step toward automating the process for reading and validating all the settings. A future change will have to move from using the "TraceSettings" and utilizing these directly. But I wanted to do baby steps (and this is a pretty sizeable baby step).
|
The android dynamic settings are not working. Taking a stab at fixing them with MarkY-LunarG#11 |
MarkY-LunarG
force-pushed
the
marky-improve-settings-capture
branch
from
716e356 to
7070239
Compare
Add a JSON for defining all GFXReconstruct settings in a single location which will eventually be used to generate all locations those settings are used. We're moving to this because we've had issues where settings are updated in one location and not another.
Generate the Vulkan layer manifest file settings using the content from the new settings.json file. Also, verify that the JSON is properly formatted before generating anything.
Generate the vk_layer_settings.txt file used by the layer that sets all the values to defaults but can be overwritten by the user.
Generate the information in the USAGE markdown docs regarding the settings since some of the settings info was out of date.
Generate a structure containing all the settings that have been defined.
Generate a settings manager which will manage loading the capture settings from environment variables or the layer settings file.
This change switches the initialization and reading of settings to the new SettingsManager class and uses the generated settings structure to read the settings and then initialize the internal capture settings structs.
The dynamic settings should only be enabled/disabled if they were intentionally set. Otherwise, we won't even create the capture file.
This adds an option to CMake to generate the settings if either the settings.json or the generate_settings.py files have changed for a desktop build (non-Android). To enable this, define the following during CMake generation: `GENERATE_SETTINGS_OPTIONS`.
Remove unused functions and add comments to describe functional usage.
Make CaptureSettings a direct part of CommonCaptureManager.
Modify the capture settings to be owned by the CommonCaptureManager. This allows a cleaner single ownership design.
MarkY-LunarG
force-pushed
the
marky-improve-settings-capture
branch
from
d34e681 to
6a48775
Compare
MarkY-LunarG
force-pushed
the
marky-improve-settings-capture
branch
from
5083a49 to
4fad3ab
Compare
6 participants
This change starts moving to a code generated process for all settings. This portion of the change introduces the generation process and provides details for the capture-based settings.
The settings are generated using scripts/contents in the
scripts/settings_optionsfolder. They are generated using thegenerate_settings.pyPython script in that folder which looks at the settings defined in thesettings.jsonfile. That file is also compared against thesettings_schema.jsonto validate it prior to executing the code/doc generation process.From these files, the following items are updated/generated: