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

expose node position in C API #293

Merged
merged 1 commit into from
Dec 2, 2023
Merged

Conversation

Kiyoshika
Copy link
Member

This is a small extension to the C API to expose the node position. After this is merged the user agent can be changed to use positions instead of the default vertical-stack layout

node: *const Node,
node_data: *mut CTextNode,
) {
pub unsafe extern "C" fn gosub_render_tree_get_node_data(node: *const Node, c_node: *mut CNode) {
Copy link
Member Author

Choose a reason for hiding this comment

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

new struct CNode introduced to more properly wrap the node_t struct from the C API. This is more generic than the previous CTextNode when we add more types

Comment on lines +28 to +31
// TODO: it'll be good at some point in the future to have the
// margins to compute the expected_y position instead of manually
// doing math. This will make the tests more robust if we change
// margins/etc. in the engine.
Copy link
Member Author

Choose a reason for hiding this comment

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

converting this to issue

@Kiyoshika Kiyoshika requested a review from a team December 1, 2023 05:36
Comment on lines +29 to +31
pub fn new_root() -> Self {
Self::default()
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not really needed or used, but leaving it here "for completion" (typically the root node is always skipped in the iterator because it's not really useful)

Copy link
Member Author

@Kiyoshika Kiyoshika Dec 1, 2023

Choose a reason for hiding this comment

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

Although now that I think about it, for the purpose of the RenderTree, the root node is technically not a visible node so should probably be skipped in the iterator itself anyways

Comment on lines +31 to +32
double render_tree_node_get_x(const struct node_t *node);
double render_tree_node_get_y(const struct node_t *node);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it makes sense to attach these functions to the node_t structs. The only problem I'd see with attaching them to the node_t is, that it could make it harder to call them.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean exactly? These already take a node_t

Copy link
Member

Choose a reason for hiding this comment

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

Ah, what i meant is a c++ only feature thought, i did this once in C. Also, i thought i had seen this once in a c library.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, yea c++ allows methods in structs since they’re a special case of a class in that case

Copy link
Member Author

Choose a reason for hiding this comment

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

Some people put function pointers in structs but I only do that if it’s really necessary

Copy link
Member

Choose a reason for hiding this comment

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

Ya, I thought this was possible because C had no classes. But probably when c had this, there would be no c++

Copy link
Member

Choose a reason for hiding this comment

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

Some people put function pointers in structs, but I only do that if it’s really necessary

Ah, probably I was a bit confused with this then.

@Kiyoshika Kiyoshika merged commit 7510671 into main Dec 2, 2023
4 checks passed
@Kiyoshika Kiyoshika deleted the feat/expose-position-in-c-api branch December 2, 2023 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants