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

PR: Improve overall code consistency. #5

Merged
merged 24 commits into from Aug 3, 2015
Merged

PR: Improve overall code consistency. #5

merged 24 commits into from Aug 3, 2015

Conversation

KelSolaar
Copy link

This PR is NOT ready to merge but more a draft and discussion support as I was trying to improve consistency in the naming of the code objects:

Names Meaning

There is a mix of create_* and generate_* definitions. For consistency I think it would be probably a good idea to have the create prefixes associated with any object creation within the code and generate with anything that produce a file as a result.

Names Case

This is where the big chunk of work is for me, taking a look at a the most important cherry picked definitions and their arguments:

  • colorspaces
    • aces.py
      • ACES_AP1_TO_AP0

      • ACES_AP0_TO_AP1

      • ACES_AP0_TO_XYZ

      • ACES_XYZ_TO_AP0

      • create_ACES

      • create_ACEScc

        • aces_ctl_directory
        • lut_directory
        • lut_resolution_1d
        • cleanup
        • name
        • min_value
        • max_value
        • input_scale
      • create_ACESproxy

      • create_ACEScg

      • create_ADX

      • create_generic_log

      • create_Dolby_PQ

        -aces_CTL_directory
        -lut_directory
        -lut_resolution_1d
        -cleanup
        -name
        -aliases
        -min_value
        -max_value
        -input_scale

      • create_Dolby_PQ_scaled

      • create_ACES_LMT

      • create_LMTs

      • create_ACES_RRT_plus_ODT

      • create_ODTs

      • get_transform_info

      • get_ODTs_info

      • get_LMTs_info

      • create_colorspaces

    • arri.py
      • create_log_c
      • create_colorspaces
    • canon.py
      • create_c_log
      • create_colorspaces
    • general.py
      • create_matrix_colorspace
      • create_transfer_colorspace
      • create_matrix_plus_transfer_colorspace
      • transfer_function_sRGB_to_linear
      • transfer_function_Rec709_to_linear
      • transfer_function_Rec2020_10bit_to_linear
      • transfer_function_Rec2020_12bit_to_linear
      • transfer_function_Rec1886_to_linear
      • create_colorspaces
      • create_raw
    • gopro.py
      • create_protune
      • create_colorspaces
    • panasonic.py
      • create_v_log
      • create_colorspaces
    • red.py
      • create_RED_log_film
      • create_colorspaces
    • sony.py
      • create_s_log
      • create_colorspaces
  • config.py
    • create_ocio_transform
    • add_colorspace_aliases
    • add_look
    • add_looks_to_views
    • generate_config_directory
    • create_config_data
    • set_config_roles
    • create_config
    • write_config
    • generate_baked_LUTs
      • odt_info
      • shaper_name
      • baked_directory
      • config_path
      • lut_resolution_3d
      • lut_resolution_shaper
      • prefix
    • generate_config
    • main
  • lut.py
    • generate_1d_LUT_image
    • write_SPI_1d
    • write_CSP_1d
    • write_CTL_1d
    • write_1d
    • generate_1d_LUT_from_image
    • generate_3d_LUT_image
    • generate_3d_LUT_from_image
    • apply_CTL_to_image
    • convert_bit_depth
    • generate_1d_LUT_from_CTL
    • correct_LUT_image
    • generate_3d_LUT_from_CTL
    • main

There is a huge margin for improvement, for example we have mix of:

  • lut & LUT
  • ctl & CTL
  • s_log & RED
  • Rec2020 & protune

PEP 8 recommend having everything lower cased, it would remove any consistency problems however I quite like having definitions such as create_ACESproxy instead of create_aces_proxy. We do use upper cased often in Colour without it being an issue provided it is consistent. Maybe what we can do for now it allow upper case in definitions names, and use lower cased arguments.

Which leads me to a question regarding parameters for the command line binaries, we should have better naming consistency there too:

  • --lutResolution1d
  • --lutResolution3d
  • --dontBakeSecondaryLUTs

Is it also maybe worth dropping the camel cased names for something along those lines, which is probably more usual in command line tools:

  • --lut-resolution-1d
  • --lut-resolution-3d
  • --dont-bake-secondary-luts

@KelSolaar
Copy link
Author

I'm right now working in a parallel branch: https://github.com/colour-science/OpenColorIO-Configs/tree/feature/code_style_rebase and will push back to that one once I have redone all the work. I'm taking a much more grained approach that will make changes easy to review. My comments above still apply though :)

@KelSolaar KelSolaar changed the title PR: Improve overall naming consistency. PR: Improve overall code consistency. Jul 31, 2015
@KelSolaar
Copy link
Author

Here we go, still a lot to do though!

@hpd
Copy link
Owner

hpd commented Jul 31, 2015

I've added comments into the various commits but for the most part, they are pretty minor suggestions. Overall, this is a huge improvement. Thank you!

hpd added a commit that referenced this pull request Aug 3, 2015
PR: Improve overall code consistency.
@hpd hpd merged commit 7d36b2a into hpd:master Aug 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants