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

Added configurable delimiters for parseColorSpaceFromString() #381

Closed
wants to merge 1 commit into from

Conversation

mike1158
Copy link

I went ahead and made a pull req. based on the discussion in ocio-dev (https://groups.google.com/forum/#!topic/ocio-dev/dfOxq8Nanl8)

@Shootfast
Copy link
Contributor

Hi, just wanted to say I haven't forgotten this. I'll run through the changes soon and hopefully merge this. Sorry for the slowness!

@mplec
Copy link
Contributor

mplec commented Nov 3, 2015

Any life left in this one? We are running into the same problem and this would be a big help. We can patch locally but it seemed like there was plenty of support in the original discussion (https://groups.google.com/forum/#!searchin/ocio-dev/strictparsing/ocio-dev/dfOxq8Nanl8/TgdFL-2q6c8J). Was there any reason not to merge this?

Thanks,
Matt

@malcolmhumphreys
Copy link
Contributor

Sorry that it took so long to get back to this.

Do you think that delimiters would be used else where in OCIO? I would either lean towards being more verbose in the API name or use YAML markup to be an actual list vs a packed string.

@mike1158 I hope you still care about this. @lgritz / @mplec it would be good to get your thoughts on this as well.

@mplec
Copy link
Contributor

mplec commented Sep 17, 2016

I don't have specific use cases for using delimiters elsewhere but it's not hard to imagine parsing other ocio config-defined names from strings, maybe display names from a string, something like that. But I'm just speculating. Splitting out the check for being delimited that was implemented in the PR to a function like isDelimited(substring, string) might be cleaner and more likely to get reused instead of unintentionally reimplemented?

On the YAML list vs packed string, I don't have any strong feelings. However a list would future proof if the delimiter ever needs to be more than a single character. That seems worth something.

Another question that popped into my head on re-reading is do we care about different delimiters before and after? I can't think of a case where I need that now but maybe someone else can? However at some point you're trying to accommodate everyone's pipeline quirks in a library convenience function that should be simple and cover 80% reliably and beyond that we can just do our own pipeline-specific parsing knowing how we name things, as we're doing now.

@malcolmhumphreys
Copy link
Contributor

Please see new pull request #413

devernay added a commit to MrKepzie/openfx-io that referenced this pull request Feb 1, 2017
@scoopxyz scoopxyz closed this Oct 27, 2017
@scoopxyz scoopxyz reopened this Oct 27, 2017
devernay added a commit to NatronGitHub/openfx-io that referenced this pull request May 2, 2019
@michdolan
Copy link
Collaborator

Curious if we feel like the new OCIO v2 file rules section solves this adequately. It's a different solution, but since it supports wildcards and regex it is more flexible. @hodoulp @doug-walker do you think it would be useful to support a special ${name} variable or similar in file rules regex strings to imply the placement of a color space name, rather than needing to have a pattern that accounts for every color space naming convention?

@doug-walker
Copy link
Collaborator

It's not clear why this PR is still open given that it was superseded by PR #413. Should we close this one? Michael, I will respond more fully to your question in the newer PR.

@michdolan
Copy link
Collaborator

Closing to continue discussion on how this could be realized in OCIO v2 in PR #413 since color space parsing functionality has been rewritten since this was opened.

@michdolan michdolan closed this Apr 21, 2021
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

7 participants