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

Print warning once when loading an image from res:// path #39396

Closed
wants to merge 1 commit into from
Closed

Print warning once when loading an image from res:// path #39396

wants to merge 1 commit into from

Conversation

Xrayez
Copy link
Contributor

@Xrayez Xrayez commented Jun 8, 2020

Closes #24222.

I write unit tests for image processing methods and run them via command line. Unfortunately, the output is polluted with the warning messages for each and every image.load(), often "shadowing" any error messages:

* test_image_resize_hqx2_rgb
WARNING: load: Loaded resource as image file, this will not work on export: 'res://rect_rgb.png'. Instead, import the image file as an Image resource and load it normally as a resource.
   At: core/image.cpp:1889.
* test_image_resize_hqx3_rgba
WARNING: load: Loaded resource as image file, this will not work on export: 'res://rect_rgba.png'. Instead, import the image file as an Image resource and load it normally as a resource.
   At: core/image.cpp:1889.
* test_image_rotate
WARNING: load: Loaded resource as image file, this will not work on export: 'res://rect_rgb.png'. Instead, import the image file as an Image resource and load it normally as a resource.
   At: core/image.cpp:1889.
* test_image_rotate_90_cw
WARNING: load: Loaded resource as image file, this will not work on export: 'res://rect_rgba.png'. Instead, import the image file as an Image resource and load it normally as a resource.
   At: core/image.cpp:1889.
* test_image_rotate_90_ccw
WARNING: load: Loaded resource as image file, this will not work on export: 'res://rect_rgba.png'. Instead, import the image file as an Image resource and load it normally as a resource.
   At: core/image.cpp:1889.
* test_image_rotate_180_grayscale
WARNING: load: Loaded resource as image file, this will not work on export: 'res://rect_grayscale.png'. Instead, import the image file as an Image resource and load it normally as a resource.
   At: core/image.cpp:1889.

This PR makes it so that the warning is printed only once during the lifetime of Godot instance.

@Calinou Calinou added cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement topic:core labels Jun 8, 2020
@Calinou Calinou added this to the 4.0 milestone Jun 8, 2020
@akien-mga
Copy link
Member

The warning message includes the path, so it's relevant to print it for each affected path, so IMO this change is not a good one.

This warning tells you that your tests are doing something wrong, so either your tests should be changed, or this warning is maybe wrong itself.

@Xrayez
Copy link
Contributor Author

Xrayez commented Jun 9, 2020

I've already initially loaded the images from the imported textures as:

var texture = preload("res://rect_rgba.png")
var input = texture.get_data()
# ... and then perhaps `image.duplicate()` to avoid operating on the same image data...

instead of:

var input = Image.new()
input.load("res://rect_rgba.png")

The problem with the former approach is that they must be imported. I run tests with Travis CI command line (scons platform=server). In order to import those images, I must:

  1. Run the binary once without running any tests with the --editor --quit option.
  2. Actually run the tests with imported images.

In fact, I've already tried this approach. The first step (1) causes various crashes. Likely due to

  1. When certain modules are disabled (I disable compiling most of the Godot modules to increase the build times by like 10 minutes), and the crashes seem to be related to HeightMapShape error with disabled Bullet module on project startup #32217 (3.2) (please don't ask me to give up these time saving gains). 🙂
  2. There's no video driver available on server builds which might cause crashes in some cases (?).
  3. There's just some editor stuff which crashes on server builds.

Another workaround I've tried is to simply version the .import folder so that the engine doesn't need to reimport it. Unfortunately some tests still kept failing. And yeah I personally don't want to choose this poison either. 🙂

So, I wouldn't want to rely on the import process for the "raw" image data to get tested.

P.S. I find the warning too verbose. That's something which belongs to documentation IMO. There seems to be no way to control the verbosity of warnings (or the severity of them). The message is more like informational. I think it's enough for users to read this message once, and it's easy to find all the .load("res://") references if users choose to make this change, which is only relevant when you export the project (also given the fact that you can distribute a project without exporting).

@Xrayez
Copy link
Contributor Author

Xrayez commented Jun 9, 2020

Well I've figured a nice workaround to this problem, you just have to explicitly globalize the path:

static func image_load_no_warning(filepath):
    var image = Image.new()
    image.load(ProjectSettings.globalize_path(filepath))
    return image

Feel free to close this. But it would be nice to:

  1. Have informational type of messages (INFO_PRINT, just like WARN_PRING, ERR_PRINT), so informational ones can be somehow disabled. WARN_PRINT_ONCE is the closest to INFO_PRINT which this PR aims for.
  2. Have an option to re-import assets as a command line option (AFAIK this is already the case when you --export the project, so might just require some tinkering).

@Calinou
Copy link
Member

Calinou commented Jun 9, 2020

Have an option to re-import assets as a command line option (AFAIK this is already the case when you --export the project, so might just require some tinkering).

Indeed, you can use --export-pack and not bother about the generated PCK. We should probably have an --import CLI argument to make this more explicit, without generating any stray files.

@realkotob
Copy link
Contributor

realkotob commented Jun 12, 2020

Slightly offtopic, but I'd love having an --import CLI option, it would allow for better optimized CI/CD scripts when targeting multiple platforms. (See abarichello/godot-ci#11)

@Xrayez
Copy link
Contributor Author

Xrayez commented Jun 17, 2020

Continuing the import problem, I stumbled upon a limitation/fix in WAT: watplugin/wat@b2fe586. @CodeDarigan do you have anything to add regarding what I've described in #39396 (comment)?

@AlexDarigan
Copy link

AlexDarigan commented Jun 20, 2020

@Xrayez For a start I don't even think the files get re-imported doing that. They're just ignored but all of the tests work fine doing it.

This (image below) from https://godotengine.org/article/core-refactoring-progress-report-2 may solve the issue but it's probably impossible to backport.

image might solve the problem in future (note it says server is a work around so I might build a docker image and test with that instead).

Edit: Seems the Server doesn't recognize the EditorPlugin.

@Xrayez
Copy link
Contributor Author

Xrayez commented Jul 1, 2020

No clear consensus and I've already found a workaround, so closing.

Things to work on:

  • need better support for command line importing of assets.
  • make server work alright with editor stuff.
  • would be nice to have informational type of messages (like INFO_PRINT over WARN_PRINT).

@Xrayez Xrayez closed this Jul 1, 2020
@Xrayez Xrayez deleted the image-load-warn-once branch July 1, 2020 11:07
@KoBeWi KoBeWi added the archived label Jul 1, 2020
@KoBeWi KoBeWi removed this from the 4.0 milestone Jul 1, 2020
@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jul 3, 2020
@akien-mga akien-mga modified the milestones: 3.2, 4.0 Jul 3, 2020
@Xrayez Xrayez restored the image-load-warn-once branch September 29, 2020 14:54
@Xrayez
Copy link
Contributor Author

Xrayez commented Sep 29, 2020

Reopening because I've stumbled upon a previously reported issue which may objectively justify this change now: #24222.

@Xrayez Xrayez reopened this Sep 29, 2020
@Xrayez
Copy link
Contributor Author

Xrayez commented Oct 8, 2020

Alternatively, this warning can be removed, because I've already documented this in #42412, see also #24222 (comment):

The confusion for beginners can be further mitigated by renaming Image.load to Image.load_from_file, as proposed in #16863 (comment).

Again, the use cases where this is a valid situation are:

  • editor image load and pre-processing, saving and reimporting modified images.
  • loading external images once exported, because there's no way to import images as textures at run-time.

I don't see a way to make this warning more specific to exclude those situations.

@Xrayez
Copy link
Contributor Author

Xrayez commented Oct 28, 2020

Closing in favor to #42653.

@Xrayez Xrayez closed this Oct 28, 2020
@Xrayez Xrayez deleted the image-load-warn-once branch July 31, 2021 10:48
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.

Getting "Loaded resource as image file" warning, but the code is legit because it runs in editor
6 participants