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 CanvasItem::get_canvas_layer_node() #87095

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

TML233
Copy link
Contributor

@TML233 TML233 commented Jan 12, 2024

  • Added CanvasLayer::get_canvas_layer_node, which returns the CanvasLayer node the CanvasItem is in. Returns nullptr when the CanvasItem is not in a CanvasLayer node.

Fix godotengine/godot-proposals#8863

@Sauermann
Copy link
Contributor

Sauermann commented Jan 12, 2024

Please edit in your opening post the last line and change it to:
fix godotengine/godot-proposals#8863

This change will link this PR to that proposal and causes github to automatically close the proposal, when this PR gets merged. These keywords are explained in https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests.

@TML233 TML233 force-pushed the expose_get_canvas_layer branch from dbfc368 to e676541 Compare January 12, 2024 08:30
@TML233
Copy link
Contributor Author

TML233 commented Jan 12, 2024

Please edit in your opening post the last line and change it to: fix godotengine/godot-proposals#8863

This change will link this PR to that proposal and causes github to automatically close the proposal, when this PR gets merged. These keywords are explained in https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests.

Thank you! I've fixed both issues you mentioned.

doc/classes/CanvasItem.xml Outdated Show resolved Hide resolved
doc/classes/CanvasItem.xml Outdated Show resolved Hide resolved
doc/classes/CanvasItem.xml Outdated Show resolved Hide resolved
@TML233 TML233 force-pushed the expose_get_canvas_layer branch 2 times, most recently from c0cd35e to d350bcd Compare January 12, 2024 11:04
Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

I'm not familiar with ERR_READ_THREAD_GUARD_V, but otherwise the PR changes look correct.

@akien-mga akien-mga changed the title Expose CanvasLayer::get_canvas_layer() & Add get_canvas_layer_node() Expose CanvasLayer::get_canvas_layer() & add get_canvas_layer_node() Feb 14, 2024
@akien-mga akien-mga changed the title Expose CanvasLayer::get_canvas_layer() & add get_canvas_layer_node() Expose CanvasItem::get_canvas_layer() & add get_canvas_layer_node() Feb 14, 2024
@groud
Copy link
Member

groud commented Feb 14, 2024

I think I am ok with exposing the node, as there seem to be no way to retrieve it easily otherwise.

I'd, however, avoid exposing get_canvas_layer. It bloating the API for something that could easily be done manually. See the function:

int CanvasItem::get_canvas_layer() const {
	ERR_READ_THREAD_GUARD_V(0);
	if (canvas_layer) {
		return canvas_layer->get_layer();
	} else {
		return 0;
	}
}

Also, I think the use case is not common enough to justify exposing get_canvas_layer.

@TML233
Copy link
Contributor Author

TML233 commented Feb 14, 2024

I think I am ok with exposing the node, as there seem to be no way to retrieve it easily otherwise.

I'd, however, avoid exposing get_canvas_layer. It bloating the API for something that could easily be done manually. See the function:


int CanvasItem::get_canvas_layer() const {

	ERR_READ_THREAD_GUARD_V(0);

	if (canvas_layer) {

		return canvas_layer->get_layer();

	} else {

		return 0;

	}

}

Also, I think the use case is not common enough to justify exposing get_canvas_layer.

Okay, I'll de-expose get_canvas_layer but keep get_canvas_layer_node.

doc/classes/CanvasItem.xml Outdated Show resolved Hide resolved
@TML233 TML233 force-pushed the expose_get_canvas_layer branch from d350bcd to 9a7c3d6 Compare February 18, 2024 13:09
@Mickeon
Copy link
Contributor

Mickeon commented Feb 18, 2024

The commit could use a different name since get_canvas_layer is no longer exposed.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

The documentation is completely fine, though

@AThousandShips AThousandShips changed the title Expose CanvasItem::get_canvas_layer() & add get_canvas_layer_node() Add CanvasItem::get_canvas_layer_node() Feb 18, 2024
@akien-mga akien-mga force-pushed the expose_get_canvas_layer branch from 9a7c3d6 to 6a17f2a Compare February 20, 2024 17:15
@akien-mga akien-mga merged commit 4d6d2d1 into godotengine:master Feb 20, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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.

Expose CanvasItem.get_canvas_layer() to GDScript/C#
6 participants