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

Added the Perspective editor #806

Merged

Conversation

Variable-ind
Copy link
Contributor

This was one of my extensions, i massively improved the original code, and added it to the core so Heavy Testing Encouraged

A Demonstration of Power:

PerspectiveEditor.mp4

@dphaldes
Copy link
Contributor

dphaldes commented Jan 6, 2023

Looks great! 😄
This is my preliminary review:

  1. Don't use a colored background for element like "Point 1". It makes a bit harder to read. Instead you can use a small icon next to it or use a small filled square.
  2. Don't use colored foreground for buttons. It is distracting (red on dark bg is difficult to read). Instead use a undo point when deleting and prompt user with a warning dialog if it is something major

one more smaller change. change = in X and Y to a colon (:) to maintain consistency with other panels

@Variable-ind
Copy link
Contributor Author

Looks great! smile This is my preliminary review:

1. Don't use a colored background for element like "Point 1". It makes a bit harder to read. Instead you can use a small icon next to it or use a small filled square.

2. Don't use colored foreground for buttons. It is distracting (red on dark bg is difficult to read). Instead use a undo point when deleting and prompt user with a warning dialog if it is something major

one more smaller change. change = in X and Y to a colon (:) to maintain consistency with other panels

How about this?
Screenshot from 2023-01-06 15-21-28

@dphaldes
Copy link
Contributor

dphaldes commented Jan 6, 2023

Looks better! We should probably add icons to buttons throughout the editor (Little side project for future 😄 )

image
It will be better if these line elements were added vertically instead of horizontally.
Also can these elements be made into collapsible widgets like Godot editor instead of popups ?
image

And move the delete button to the very bottom so that its more prominent and people like me don't misclick it when trying to change color 😅

Copy link
Member

@OverloadedOrama OverloadedOrama left a comment

Choose a reason for hiding this comment

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

image
There seems to be an issue with the Line collapsible containers. Expanding the Line1 container in the screenshot, creates another empty collapsible container directly below it, which doesn't seem to do anything. This also happens for every line container, but not in any of the other collapsible containers, like "Lines" or "Point: 1".

I also think that the perspective editor should probably be next to the canvas, but hidden by default, as most people will probably not need it. If you're having difficulties changing the Layout resources, I can do it for you.

Apart from that, I'm not 100% sure about the current status of the UI, but we can make further changes after the PR is merged.

@Variable-ind
Copy link
Contributor Author

Done

image There seems to be an issue with the Line collapsible containers. Expanding the Line1 container in the screenshot, creates another empty collapsible container directly below it, which doesn't seem to do anything. This also happens for every line container, but not in any of the other collapsible containers, like "Lines" or "Point: 1".

I also think that the perspective editor should probably be next to the canvas, but hidden by default, as most people will probably not need it. If you're having difficulties changing the Layout resources, I can do it for you.

Apart from that, I'm not 100% sure about the current status of the UI, but we can make further changes after the PR is merged.

Done

Copy link
Member

@OverloadedOrama OverloadedOrama left a comment

Choose a reason for hiding this comment

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

Fantastic work, thank you very much!

@OverloadedOrama OverloadedOrama merged commit 7307743 into Orama-Interactive:master Feb 12, 2023
@Variable-ind Variable-ind deleted the Perspective-Editor branch February 12, 2023 13:24
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