AcademySoftwareFoundation / OpenColorIO

A color management framework for visual effects and animation.
https://opencolorio.org
BSD 3-Clause "New" or "Revised" License
1.79k stars 455 forks source link

Parsing error on whitespace #527

Open sobotka opened 6 years ago

sobotka commented 6 years ago

direction:inverse fails while direction: inverse succeeds silently.

@Shootfast reports that ociocheck also fails to note the problem.

michdolan commented 6 years ago

According to the YAML spec, keys are separated from values by a colon+space. In this case, direction:inverse is being interpreted as a key. OCIO's YAML parser handles unknown keys by logging a warning, printed to stderr.

If for some reason your OCIO logging level (set either via the OCIO_LOGGING_LEVEL environment variable, or OCIO::SetLoggingLevel()) is set to LOGGING_LEVEL_NONE or 0, that would hide this warning. The default is LOGGING_LEVEL_INFO or 2, which will log warnings.

I ran a quick test in Nuke to verify this. My direction:inverse yielded the following in the console: [OpenColorIO Warning]: Unknown key in FileTransform: 'direction:inverse'. (line 808, column 78)

The warning should occur anytime the config is read, including with ociocheck. Where were you loading the config and not seeing the warning? Could you also double check your logging level?

Perhaps we should force a more verbose logging level in ociocheck to make sure warnings are visible despite the environment.

dbr commented 6 years ago

This should just be a syntax error - I think the laxness is coming from OCIO's own parsing of it's custom ! tags

For example, this produces a syntax error (at least in PyYAML):

colorspaces:
  - to_reference: {direction:inverse}

This is good, as there should be a space before the value. However when this is wrapped in OCIO's !<FileTransform> {...} tag or whatever, I'd expect the syntax for the {...} map to be the same, but it is more lax:

colorspaces:
  - to_reference: !<FileTransform> {direction:inverse} # should error but OCIO is fine

Might just be something like the code in here: https://github.com/imageworks/OpenColorIO/blob/f35a8ef0ac01c74b616ab0be3902155cc671f95d/src/core/OCIOYaml.cpp#L633-L650 needs to ensure all the values are map items (not sure the exact terminology)

michdolan commented 6 years ago

Good point @dbr . Also, I'm realizing the behavior I described with logging this as an unknown key pertains only to OCIO 1.0.9. With 1.1.0, the value being of type YAML::NodeType::Null supersedes the unknown key and routes through the condition you referenced:

if (second.Type() == YAML::NodeType::Null) continue;

That explains the old warning being gone now.

yaml-cpp doesn't appear to treat this map flow breakage as such (where PyYAML does). I tested this against yaml-cpp 0.3.0 and 0.5.1. I haven't tested 0.6.2 yet, which maybe should be done in general (since it removes the boost dependency). If all yaml-cpp releases treat the missing key: value colon delimiter as a null value, we can open an issue there.