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

Add property checker/metadata system #166

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Add property checker/metadata system #166

wants to merge 15 commits into from

Conversation

garyo
Copy link
Contributor

@garyo garyo commented Aug 28, 2024

  • include/ofx-props.yml is the property reference/metadata
  • scripts/gen-props.py is the checker and source generator

All props are included in the YML definition, and the script generates C++ files with all the property metadata and suite assignments. The C++ files aren't yet checked in, but you can build them easily: python scripts/gen-props.py -v and then look in include/gen_*.hxx.

* include/ofx-props.yml is the property reference/metadata
* scripts/gen-props.py is the checker and source generator

All props are included in the YML definition, and the script generates
C++ files with all the property metadata and suite assignments.

Signed-off-by: Gary Oberbrunner <[email protected]>
Also include them into prop tester plugin, to test whether they
compile properly. TODO: prop tester should use the metadata to do
better tests.

Signed-off-by: Gary Oberbrunner <[email protected]>
Copy link

@abizeauMaxon abizeauMaxon left a comment

Choose a reason for hiding this comment

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

That super! I see a lot of good thing here! Will be helpfull with time.

std::vector<const char *> values; // for enums
};

const std::vector<struct PropsMetadata> props_metadata {

Choose a reason for hiding this comment

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

Since this it auto generated, you could consider std::array<struct PropsMetadata, N=FeedByScrip>.

If this header is intended to be use more C++, you should consider having this std::array as constexpr.

maybe:

#if defined(__cplusplus)
#define CONST_SPECIFIER constexpr
#else
#define CONST_SPECIFIER const
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @abizeauMaxon -- totally agreed. As per discussion at the TSC today, I've moved these into Support/include and they are 100% C++. I used fixed-size std::arrays where possible, as well as some other updates and cleanups. More feedback will be helpful; let me know if you try it out.

int dimension;
Writable writable;
bool host_optional;
std::vector<const char *> values; // for enums

Choose a reason for hiding this comment

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

You should be consistant, if you use std::string for name, you should also use std::vector<std::string>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried now to move to char *, to avoid allocations where possible during startup. I should move to std::string_view in some places. I'll do that.


namespace OpenFX {
// Properties for property sets
const std::map<const char *, std::vector<const char *>> prop_sets {

Choose a reason for hiding this comment

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

again, here you should consider consistency, pick std::string or const char*

@@ -0,0 +1,1508 @@
########################################################################

Choose a reason for hiding this comment

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

Is this auto generated? If so maybe add it into the header

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ofx-props.yml is hand-created -- it's now the source of truth for all info about properties and property sets. That's the lion's share of the work in this PR (and probably the source of remaining bugs, where I may have gotten some stuff wrong). None of this info was machine-readable before, so I extracted it from the docs and header file comments.

The strings are the true spec, not the #define names. This also adds a
set of static_assert tests to ensure the #define names match their
strings. But doing this brought up that there are some mismatches
between the strings and their #defines. Those will be accounted for
in a following commit.

Signed-off-by: Gary Oberbrunner <[email protected]>
See `get_cname` and `find_stringname` in gen-props.py, and the new
`cname` member of the props metadata.

Signed-off-by: Gary Oberbrunner <[email protected]>
Replaced a vector with a std::array for reduced memory & startup time.
Also add static_assert checks for all action names vs. their #defines.

Signed-off-by: Gary Oberbrunner <[email protected]>
Also separate out Actions from prop sets in ofx-props.yml, for
cleanliness.

Signed-off-by: Gary Oberbrunner <[email protected]>
@garyo garyo marked this pull request as ready for review October 1, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants