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

Allow enabling tinyexr module compilation in export templates #73003

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Feb 9, 2023

After enabling the tinyexr module at build-time by passing the tinyexr_export_templates=yes SCons option, this makes it possible to use Image.save_exr() in export templates. When the module is enabled, Image.load() can also be used to load images in OpenEXR format.

When tinyexr is enabled in an export template, binary size is about 100 KB larger (stripped on Linux x86_64). As a result, this remains disabled by default.

@Calinou Calinou requested review from a team as code owners February 9, 2023 23:07
@Calinou Calinou added enhancement topic:buildsystem cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Feb 9, 2023
@Calinou Calinou modified the milestones: 4.x, 4.1 Feb 9, 2023
@Calinou Calinou force-pushed the tinyexr-allow-enable-export-templates branch from 5a23ac6 to 94a47fd Compare February 9, 2023 23:13
SConstruct Outdated
@@ -207,6 +207,13 @@ opts.Add(BoolVariable("modules_enabled_by_default", "If no, disable all modules
opts.Add(BoolVariable("no_editor_splash", "Don't use the custom splash screen for the editor", True))
opts.Add("system_certs_path", "Use this path as SSL certificates default for editor (for package maintainers)", "")
opts.Add(BoolVariable("use_precise_math_checks", "Math checks use very precise epsilon (debug option)", False))
opts.Add(
BoolVariable(
"tinyexr_export_templates",
Copy link
Member

Choose a reason for hiding this comment

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

Would it work to move this option to the tinyexr module's config.py, so we don't bloat the main script? I think modules support adding options, see the Mono module.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've pushed a second commit where I attempted this, but it fails when using target=template_release:

KeyError: 'tinyexr_export_templates':
  File "/home/hugo/Documents/Git/godotengine/godot/SConstruct", line 794:
    if config.can_build(env, selected_platform):
  File "/home/hugo/Documents/Git/godotengine/godot/modules/tinyexr/config.py", line 2:
    return env.editor_build or env["tinyexr_export_templates"]
  File "/home/hugo/.local/opt/pyston/lib/python3.8-pyston2.3/site-packages/SCons/Environment.py", line 387:
    return self._dict[key]

The option is listed in scons -h's output still.

After enabling the tinyexr module at build-time by passing the
`tinyexr_export_templates=yes` SCons option, this makes it possible to use
`Image.save_exr()` in export templates. When the module is enabled, `Image.load()`
can also be used to load images in OpenEXR format.

When tinyexr is enabled in an export template, binary size is about 100 KB larger
(stripped on Linux x86_64). As a result, this remains disabled by default.
Comment on lines +6 to +17
from SCons.Script import BoolVariable, Variables, Help

envvars = Variables()
envvars.Add(
BoolVariable(
"tinyexr_export_templates",
"Enable saving and loading OpenEXR images in export template builds (increases binary size)",
False,
)
)
envvars.Update(env)
Help(envvars.GenerateHelpText(env))
Copy link
Member

Choose a reason for hiding this comment

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

This isn't the correct way to do it, see akien-mga@e75da97 for an example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement topic:buildsystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants