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 vector property types #155

Merged
merged 8 commits into from Nov 6, 2023
Merged

Conversation

nkrisc
Copy link
Contributor

@nkrisc nkrisc commented Nov 3, 2023

Description

Added four new vector properties:

  • Vector2i
  • Vector2
  • Vector3i
  • Vector3

Added related getter and setter methods to PandoraEntity for each new property type.

Updated docs to reflect new properties.

Addressed issues

Screenshots

pandora_vector_properties

@bitbrain bitbrain added 📦 feature A new piece of functionality 🔌 property Related to property functionality. labels Nov 6, 2023
@bitbrain bitbrain modified the milestone: 1.0.0 Nov 6, 2023
Copy link
Owner

@bitbrain bitbrain left a comment

Choose a reason for hiding this comment

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

@nkrisc tests seem to fail - make sure to check the logs for the errors. You also seem to have introduced a memory leak via stray nodes. Ensure to always free up nodes you instantiate via new() that are objects.

https://mikeschulze.github.io/gdUnit3/advanced_testing/orphan/

@nkrisc
Copy link
Contributor Author

nkrisc commented Nov 6, 2023

I believe the issues, including the stray nodes, are related to the fact I used the EditorSpinSlider node which fails to instantiate since the tests are not run in the editor and that particular control is only available in the editor itself. The only objects I directly create via new() are the EditorSpinSliders, and they are added to the root node of the scene so they should be freed when the property controls are freed. Looking at the logs, it seems the stray nodes occurred during the property bar test, probably when the property scenes are initially being instantiated. But I'll have to look more closely later today.

I used this control since it's an editor plugin and to ensure consistency with vector input fields elsewhere in the editor.

However, seems using that control is essentially untestable. So the options I see:

  1. Just use a SpinBox instead and have it be different.
  2. Check if it's running in the editor context or not, and choose SpinBox or EditorSpinSlider based on that
  3. Recreate the EditorSpinSlider node (probably not worth it)

…o adhere to style. Replaced boolean operators and fixed brackets.
@nkrisc
Copy link
Contributor Author

nkrisc commented Nov 6, 2023

I replaced the EditorSpinSlider with a SpinBox to keep things simple for now and all tests pass on my end. I'll leave that as a possible future enhancement.

I also realized I wasn't using GDUnit correctly initially and that's why I didn't catch it earlier - noob mistake.

@bitbrain
Copy link
Owner

bitbrain commented Nov 6, 2023

Thank you!

@bitbrain bitbrain merged commit 7249f47 into bitbrain:godot-4.x Nov 6, 2023
3 checks passed
@nkrisc nkrisc deleted the vector_properties branch November 6, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 feature A new piece of functionality 🔌 property Related to property functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🎯 Vector property
2 participants