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

Add ImageTexture unit tests #88044

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

brennennen
Copy link
Contributor

@brennennen brennennen commented Feb 7, 2024

Added ImageTexture unit tests.

I wasn't entirely sure on how to go about testing the draw methods and some feedback around that area would be appreciated. Thanks.

Link back to #43440

@brennennen brennennen requested a review from a team as a code owner February 7, 2024 03:33
@brennennen brennennen changed the title Added ImageTexture unit tests. Feedback is appreciated. Added ImageTexture unit tests Feb 7, 2024
@Sauermann
Copy link
Contributor

Sauermann commented Feb 7, 2024

You can use the viewport texture for this and access the images pixels.
Please have a look at https://docs.godotengine.org/en/latest/classes/class_viewport.html#class-viewport-method-get-texture

For reference you find a usage of this in colorpicker:

Ref<Image> img = picker_texture_rect->get_texture()->get_image();
if (img.is_valid() && !img->is_empty()) {
Vector2 ofs = mev->get_position();
picker_color = img->get_pixel(ofs.x, ofs.y);

@AThousandShips AThousandShips added this to the 4.x milestone Feb 7, 2024
@brennennen brennennen force-pushed the image_texture_unit_tests branch 2 times, most recently from 27cddae to f507a0d Compare February 7, 2024 13:51
@Calinou
Copy link
Member

Calinou commented Feb 7, 2024

You can use the viewport texture for this and access the images pixels.

Will this work in headless mode without a GPU? CI doesn't have access to any GPU.

@Sauermann
Copy link
Contributor

Will this work in headless mode without a GPU? CI doesn't have access to any GPU.

I didn't think about this - I'm not yet familiar with the rendering server.

After looking in detail at the PR, I'm no longer sure, if my previously mentioned method can work, since no nodes are added to the scene tree.

@AThousandShips
Copy link
Member

Can confirm it if the style errors are fixed and the CI can run

@brennennen brennennen force-pushed the image_texture_unit_tests branch from f507a0d to 4d8a8fb Compare February 7, 2024 21:33
@brennennen brennennen force-pushed the image_texture_unit_tests branch 3 times, most recently from fa0b259 to 62f7e05 Compare February 8, 2024 11:34
@brennennen brennennen requested a review from kleonc February 9, 2024 23:10
@brennennen brennennen force-pushed the image_texture_unit_tests branch 2 times, most recently from ce6a92e to 2b95df2 Compare February 12, 2024 21:09
@AThousandShips AThousandShips changed the title Added ImageTexture unit tests Add ImageTexture unit tests Feb 13, 2024
Comment on lines 108 to 119
TEST_CASE("[SceneTree][ImageTexture] draw") {
Ref<Image> image = Image::load_from_file(TestUtils::get_data_path("images/icon.png"));
Ref<ImageTexture> image_texture = ImageTexture::create_from_image(image);
RID canvas = RenderingServer::get_singleton()->canvas_create();
RID canvas_item = RenderingServer::get_singleton()->canvas_item_create();
RenderingServer::get_singleton()->canvas_item_set_parent(canvas_item, canvas);
image_texture->draw(canvas_item, Point2(0.0, 0.0));
// TODO: figure out how to check to see if it was drawn. can we sample a pixel on the canvas?
// I suppose not segfaulting/crashing is a positive result that might make this worth
// keeping.
}
Copy link
Member

Choose a reason for hiding this comment

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

Note this wouldn't draw anything, the canvas would also need to be attached to a viewport.

But if tests run in headless mode (as Calinou mentioned in #88044 (comment)) then this test case seems useless to me. 🤔 Again, I'm not that familiar with the tests / not sure what's desired here hence I won't be approving/dismissing it. cc @akien-mga
(the other test cases look fine to me though)

Copy link
Member

Choose a reason for hiding this comment

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

The tests typically run as headless, but they should also work if ran with an actual rendering server and display server.

So it's worth making sure that it does something useful in non-headless mode, and if it can't work in headless, it can be made optional with some if (DisplayServer::get_singleton()->get_name() == "headless") checks.

Now I didn't check, but does headless drawing to a texture in memory actually work? If it does, that's also worth testing.

Copy link
Contributor Author

@brennennen brennennen Feb 14, 2024

Choose a reason for hiding this comment

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

Thanks for the feedback.

I'm not sure how to execute tests headless vs non-headless. When you add the "--test" flag, the editor binary doesn't actually start up, it just executes the tests and exits like the console binary. Looking at the startup code, it looks like if the "--test" argument is present, the main enters "TEST_MAIN_PARAM_OVERRIDE" and returns/exits from that before opening a windows context or running the actual proper engine main.

Things I've tried:

  • DisplayServer::get_singleton()->get_name() returns "mock"
  • DisplayServer::get_singleton()->window_can_draw() returns false
  • Adding proper viewport initialization code (see below), errors out on "RenderDummy" calls to access a null pointer viewport texture.
  • Accessing "RSG::viewport->get_total_objects_drawn()" and similar metrics functions, they all always return 0.

Unless anyone has any other ideas, I'm on board to just delete the draw test case.

TEST_CASE("[SceneTree][ImageTexture] draw") {
	print_line(DisplayServer::get_singleton()->get_name()); // prints "mock"

	RID scenario = RS::get_singleton()->scenario_create();
	RID viewport = RS::get_singleton()->viewport_create();
	RS::get_singleton()->viewport_set_update_mode(viewport, RS::VIEWPORT_UPDATE_DISABLED);
	RS::get_singleton()->viewport_set_scenario(viewport, scenario);
	RS::get_singleton()->viewport_set_size(viewport, 128, 128);
	RS::get_singleton()->viewport_set_transparent_background(viewport, true);
	RS::get_singleton()->viewport_set_active(viewport, true);

	Ref<Image> image = Image::load_from_file(TestUtils::get_data_path("images/icon.png"));
	Ref<ImageTexture> image_texture = ImageTexture::create_from_image(image);
	RID canvas = RenderingServer::get_singleton()->canvas_create();
	RID canvas_item = RenderingServer::get_singleton()->canvas_item_create();
	RenderingServer::get_singleton()->canvas_item_set_parent(canvas_item, canvas);
	image_texture->draw(canvas_item, Point2(0.0, 0.0));
	
	print_line("get_total_objects_drawn: ", RSG::viewport->get_total_objects_drawn()); // prints 0
	print_line("get_total_primitives_drawn: ", RSG::viewport->get_total_primitives_drawn()); // prints 0
	print_line("get_total_draw_calls_used: ", RSG::viewport->get_total_draw_calls_used()); // prints 0
	
	RS::get_singleton()->viewport_attach_canvas(viewport, canvas);
	RID viewport_texture = RS::get_singleton()->viewport_get_texture(viewport);
	Ref<Image> img = RS::get_singleton()->texture_2d_get(viewport_texture); // ERROR: Parameter "t" is null.
// at: RendererDummy::TextureStorage::texture_2d_get (C:\repos\godot\servers\rendering\dummy\storage\texture_storage.h:106)
 	if (img.is_valid() && !img->is_empty()) { 
		// never reached
	} else {
		print_line("viewport texture is null/empty"); // executed every time
	}
}

Copy link
Member

Choose a reason for hiding this comment

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

Unless anyone has any other ideas, I'm on board to just delete the draw test case.

Yeah, makes sense to just remove it. If desired (and decided what's a proper way to test it) it could be always added separately in another PR.
(Sorry for the late response; I've seen your comment before but I think I've missed this specific sentence. 🤔)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All good, my comment was a bit of a wall of text. I went ahead and deleted the test case. I'll re-request a review.

Copy link
Member

Choose a reason for hiding this comment

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

I went ahead and deleted the test case. I'll re-request a review.

I've already approved the PR after the removal. So that was a pointless re-request. 🙃

tests/scene/test_image_texture.h Outdated Show resolved Hide resolved
@brennennen brennennen force-pushed the image_texture_unit_tests branch 5 times, most recently from dd9a207 to 0b6f0b9 Compare February 18, 2024 03:07
@brennennen brennennen force-pushed the image_texture_unit_tests branch from 0b6f0b9 to 6dbbc24 Compare February 19, 2024 17:15
@akien-mga akien-mga removed this from the 4.x milestone Feb 19, 2024
@akien-mga akien-mga added this to the 4.3 milestone Feb 19, 2024
@brennennen brennennen requested a review from kleonc February 19, 2024 23:19
@akien-mga akien-mga merged commit 3497abf into godotengine:master Feb 20, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

6 participants