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

Entry restyle and hexagonal booleans #273

Closed
wants to merge 4 commits into from

Conversation

DoomTas3r
Copy link
Contributor

@DoomTas3r DoomTas3r commented Oct 14, 2024

Adds a new top knob variant and a new background variant. Bottom tabs and outlines can now be hidden. Booleans are now hexagonal by default

Targeting #278 and #279

image
image

@DoomTas3r DoomTas3r marked this pull request as draft October 15, 2024 15:18
@DoomTas3r DoomTas3r marked this pull request as ready for review October 15, 2024 15:47
@DoomTas3r DoomTas3r changed the title Adds hexagons Adds variants Oct 15, 2024
@DoomTas3r DoomTas3r marked this pull request as draft October 17, 2024 23:46
Adds 8 new shapes for the background used in blocks, the bottom tab can be hidden and is shown by default, makes outlines optional and are shown by default
@DoomTas3r DoomTas3r changed the title Adds variants Background variants Oct 17, 2024
@DoomTas3r DoomTas3r marked this pull request as ready for review October 17, 2024 23:57
@manuq
Copy link
Contributor

manuq commented Oct 18, 2024

Hi @DoomTas3r. Although nice, adding variants to the blocks shape is not fixing or improving anything per se, so unlikely to be merged. But here is a proposal to improve the entry blocks and your help would be very appreciated! #278

Can you retarget this PR to make that improvement?

@manuq
Copy link
Contributor

manuq commented Oct 18, 2024

@DoomTas3r this is another issue that you can help with related to block shapes: #279

@DoomTas3r DoomTas3r marked this pull request as draft October 18, 2024 20:25
@DoomTas3r DoomTas3r changed the title Background variants Entry restyle, hexagonal booleans, and extra variants Oct 20, 2024
@DoomTas3r DoomTas3r marked this pull request as ready for review October 20, 2024 01:17
@DoomTas3r
Copy link
Contributor Author

@manuq I think this should be good. You can tell me if any errors related to this are found. The other types can also be styled but I was hesitant because confusion can be caused from overloading

@manuq
Copy link
Contributor

manuq commented Oct 21, 2024

@DoomTas3r whoa!! What you managed to do regarding shapes is amazing, thanks! I'll review the code in a bit.

Copy link
Contributor

@manuq manuq left a comment

Choose a reason for hiding this comment

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

@DoomTas3r I've been delaying this review so much, sorry! Here are a few changes requested. It works like a charm.

_panel_normal.border_color = color.darkened(0.2)
if not definition == null and definition.variant_type == Variant.Type.TYPE_BOOL:
_background.visible = true
_background.variant = 3
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use enumerators rather than integers. For example:

enum BackgroundVariant { ROUND, POINTY, ... }

@export var top_variant: int = 0:
set = _set_top_variant

## |0|, \1/, /2/, <3>, >4>, v5v, v6^, \7y, /8y
Copy link
Contributor

Choose a reason for hiding this comment

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

We really don't need the 9 shapes that the first commit adds, and they make the code very hard to read. We only need:

  • A pointy background for value blocks of variant type BOOLEAN.
  • A round ellipse for entry blocks.

if variant > 0:
# Top
if variant == 3 or variant == 4:
fill_polygon.append(Vector2(size.x - 5.0, 0.0))
Copy link
Contributor

Choose a reason for hiding this comment

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

These 5.0 are hardcoded everywhere. Please add a new "pointy width" constant:

Suggested change
fill_polygon.append(Vector2(size.x - 5.0, 0.0))
fill_polygon.append(Vector2(size.x - Constants.POINTY_WIDTH, 0.0))

@DoomTas3r DoomTas3r marked this pull request as draft November 4, 2024 20:43
@DoomTas3r DoomTas3r marked this pull request as ready for review November 5, 2024 14:17
@DoomTas3r DoomTas3r changed the title Entry restyle, hexagonal booleans, and extra variants Entry restyle and hexagonal booleans Nov 5, 2024
@DoomTas3r
Copy link
Contributor Author

@manuq Okay, the code was cleaned up. I thought maybe the Vector2 casting would be confusing, but I checked this block configuration
image
and it gave the error Parser Error: Value of type "Vector2" cannot be assigned to a variable of type "bool". which I thought was possible according to #254 and https://docs.godotengine.org/en/stable/classes/class_vector2.html but it seems that Vector2 can only be used as a bool for operations because this configuration works
image

@DoomTas3r DoomTas3r requested a review from manuq November 5, 2024 15:56
@manuq
Copy link
Contributor

manuq commented Nov 6, 2024

@DoomTas3r thanks! The PR diff is much clear now. Now you can see that the last commit reverts things added in the previous ones. The next level in contribution is to do an interactive rebase of your work using git rebase --interactive on your end. Then you could (for example) come up with two clean commits:

  • Restyle entry shape
  • Make boolean blocks shape pointy

For the sake of merging this soon, I'll do the interactive rebase myself this time.

@manuq
Copy link
Contributor

manuq commented Nov 27, 2024

I made a refactor of the current shapes to make this easier to implement #323

@manuq manuq mentioned this pull request Dec 2, 2024
@manuq
Copy link
Contributor

manuq commented Dec 5, 2024

A cleaned up version of this has been merged #325

Thanks so much for your contribution, @DoomTas3r !

@manuq manuq closed this Dec 5, 2024
@DoomTas3r DoomTas3r deleted the hexagons branch December 5, 2024 15:29
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.

2 participants