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 ability to select instrument brush and brush size in Editor UI #7399

Merged
merged 19 commits into from
Jul 18, 2023

Conversation

Districh-ru
Copy link
Collaborator

@Districh-ru Districh-ru commented Jul 9, 2023

This PR relates to #6845 and adds Map Editor UI ability to:

  • select Brush size;
  • add right click tips for Brush size;
  • select terrain/object type;
  • add right click tips for terrain/object type;
  • add text for selected terrain/object type.

fheroes2 2023-07-12 18-46-28-118 изображение

@Districh-ru Districh-ru added improvement New feature, request or improvement ui UI/GUI related stuff editor Map editor related stuff labels Jul 9, 2023
@Districh-ru Districh-ru added this to the 1.0.7 milestone Jul 9, 2023
@Districh-ru Districh-ru self-assigned this Jul 9, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

src/fheroes2/editor/editor_interface_panel.h Outdated Show resolved Hide resolved
@Districh-ru Districh-ru changed the title Add ability to select instruments and brush size in Editor UI Add ability to select instrument brush and brush size in Editor UI Jul 9, 2023
@Districh-ru Districh-ru marked this pull request as ready for review July 12, 2023 16:20
@Districh-ru Districh-ru requested a review from ihhub July 12, 2023 16:21
Copy link
Owner

@ihhub ihhub left a comment

Choose a reason for hiding this comment

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

Hi @Districh-ru , I left multiple comments here. Could you please check them?

src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
@ihhub
Copy link
Owner

ihhub commented Jul 13, 2023

Hi @Districh-ru , I think creating separate functions to return text instead of the proposed container would be even better solution.

@Districh-ru
Copy link
Collaborator Author

Hi @Districh-ru , I think creating separate functions to return text instead of the proposed container would be even better solution.

Hi, @ihhub, sure. Initially I was thinking about making such function.. :)
I'll address your comments a little later.

@Districh-ru Districh-ru marked this pull request as draft July 13, 2023 14:49
@Districh-ru Districh-ru marked this pull request as ready for review July 13, 2023 18:10
Copy link
Owner

@ihhub ihhub left a comment

Choose a reason for hiding this comment

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

Hi @Districh-ru , I added several comments here. Could you please check them when you have time and see if them make sense?

src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
@Districh-ru Districh-ru marked this pull request as draft July 14, 2023 17:04
@Districh-ru Districh-ru marked this pull request as ready for review July 14, 2023 18:21
@Districh-ru Districh-ru requested a review from ihhub July 14, 2023 18:21
Copy link
Owner

@ihhub ihhub left a comment

Choose a reason for hiding this comment

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

Hi @Districh-ru , I left few more comments in this pull request. My apologies if it takes too much time. Could you please take a look at the comment again and validate if they make sense?

src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_interface_panel.h Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_interface_panel.h Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
@Districh-ru Districh-ru requested a review from ihhub July 15, 2023 06:59
src/fheroes2/editor/editor_interface_panel.cpp Outdated Show resolved Hide resolved
@ihhub
Copy link
Owner

ihhub commented Jul 16, 2023

@drevoborod , could you please test these changes? You will need a debug version of the game. Press E in the Main Menu to open the Editor.

@drevoborod
Copy link
Contributor

Checked the following:

  • select terrain/object type;
  • select Brush size;
  • add right click tips for Brush size;
  • add right click tips for terrain/object type;
  • add text for selected terrain/object type.

@Districh-ru , no issues, but:

  1. is it OK that no type of selected brush actually allows to draw anything? And same to object selection - actual objects selection window does not appear on clicking to corresponding buttons?
  2. Is this text correct? Actually this unlimited size brush can be used not for cleaning but for creation of large size areas.
    изображение
    And in formal editor2.exe there is other text:
    изображение
    Looks like you'we taken this text from the case when eraser is selected. In formal editor2.exe these two texts differ depending on what is selected: some terrain or the eraser, and in current editor's revision there is no separate text.
  3. By the way, should all these texts be same as in formal editor? Some of them differ.
  4. Seems like in these messages there should be a non-breaking space between an article and a noun:
    изображение
    изображение
    изображение
    изображение
    изображение

@ihhub
Copy link
Owner

ihhub commented Jul 17, 2023

Hi @drevoborod , answering your remarks:

  1. Yes, it is not implemented as a part of this pull request.
  2. Good point. I think we can address it in a separate PR.
  3. No. We don't want to replicate everything as it is. We want better :)
  4. It's the way how text rendering works. We might change texts for these dialogs in the future.

@Districh-ru
Copy link
Collaborator Author

Hi @ihhub and @drevoborod, I'll address points 2 and 4 today a little later.

@Districh-ru
Copy link
Collaborator Author

Hi @drevoborod, the brush size texts are now separated to draw/erase and a/an article in object type descriptions is moved to a new line.
Could you please check these changes when you have time.

@drevoborod
Copy link
Contributor

the brush size texts are now separated to draw/erase and a/an article in object type descriptions is moved to a new line.

Checked it. Now it seems OK for me. But I noticed another minor issue. It's up to you to fix it or not:
изображение
I suggest to place "4 by 4" and similar texts on the same line, so possibly it could be better to leave them on the first line.

@drevoborod
Copy link
Contributor

Checked last changes - for me it looks OK now.

@Districh-ru Districh-ru requested a review from ihhub July 17, 2023 19:42
@ihhub ihhub merged commit 820e46c into ihhub:master Jul 18, 2023
@ihhub
Copy link
Owner

ihhub commented Jul 18, 2023

@Districh-ru , well done!

@Districh-ru Districh-ru deleted the editor_instruments_ui branch July 18, 2023 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor Map editor related stuff improvement New feature, request or improvement ui UI/GUI related stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants