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 a tab showing the production summary #145

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

veger
Copy link
Contributor

@veger veger commented Jan 2, 2022

I tried to reuse the existing UI of YAFC, so I based the whole thing on a ProjectPageContents and a ProjectPageView, so it would be recognized and fit into the MainScreen (tabs) and serialization.
The UI of the new 'Summary' tab is ugly, I tried making it nicer, but I found it hard to understand the ImGui and its features and how to use them... So I guess this could be improved separately from this PR?

YAFC-production-summary

I also fixed some issues I found while implementing the Summary tab:

  • calculating DatGrid width when header is not used (I found the header useless here)
  • Don't use 'weird' output of EncapsulateRect (at least I don't understand the output of this function...) as lastRect, but the actual rect of the (old) state. (So the DataGrid size can be properly propagated to the ScrollArea
  • Fixed use ScrollArea naming and exposed the parameters to enable the horizontal scrollbar. (:warning: the horizontal scrollbar is not clickable, off by 1.0f on vertical axis in the click detection, which I was not able to fix...)
  • Some other (minor) things

Related to #123

@ShadowTheAge
Copy link
Owner

Thanks for the contribution

Yafc (with its serialization, etc) was made with possible plugins in mind and this is a good candidate for a plugin (as is: It doesn't change much, mostly adding new stuff)

It is hard to review what has to be changed because all of the formatting noise (clearly my formatter is configured differently)

@ShadowTheAge
Copy link
Owner

ShadowTheAge commented Jan 3, 2022

Also, I think instead of automatically reading all the tabs, tabs should be manually added to this summary page. This way you can have multiple summaries, and also there will be no "test" tabs there, and also you can add a multiplier to a page, as in "how much of these is built".

Pages have guids, guids should be persistent and can be serialized.

@ShadowTheAge
Copy link
Owner

Also all the calculations should be done on page activation, not on project loading. So, if this page needs all other pages to be solved, it should run these solvers in its own "solve" instead of solving all the pages on a project load.

@veger
Copy link
Contributor Author

veger commented Jan 3, 2022

It is hard to review what has to be changed because all of the formatting noise (clearly my formatter is configured differently)

I guess so... 😢
GitHub diffs can ignore whitespace changes with ?w=1 -> https://github.com/ShadowTheAge/yafc/pull/145/files?w=1
That reduces most of the noise.

I also (tried) to add the different features/fixes/steps in separate commits, to (try to) ease reviewing.

Also, I think instead of automatically reading all the tabs, tabs should be manually added to this summary page. This way you can have multiple summaries, and also there will be no "test" tabs there, and also you can add a multiplier to a page, as in "how much of these is built".

Agreed, this was/is on my to-do list, but I thought this PR is already big enough as it is. (I guess it mainly requires some UI to select pages to be included)

Also all the calculations should be done on page activation, not on project loading. So, if this page needs all other pages to be solved, it should run these solvers in its own "solve" instead of solving all the pages on a project load.

Makes sense, this will be more easy to do when Summary contains a list of guids of the pages it should show... So again, it will increase the PR by quite a bit

YAFCui/ImGui/ImGuiLayout.cs Outdated Show resolved Hide resolved
@veger
Copy link
Contributor Author

veger commented Jan 3, 2022

By spending another 2 hours on the ImGui to figure it out to solve the issue I found above, I decided not to add the 'manually add tabs to a summary' idea. It will take me loads of time due to my struggle understanding the ImGui class. (I already found this will building this feature, and while I tried to beautify the GUI of the Summary tab).

For me I can select the tabs by having them in the tab bar or not and 1 Summary is enough to align inputs and outputs of the different tabs. So for me this PR serves my (personal) purpose. And spending ~a week of my free time on this is enough... (the factory did not grow at all during this time 😞 )

So feel free to accept/deny this PR (I am still willing to fix issues, solve bugs, etc to get it accepted), but feature-wise this is my limit 😉

@ShadowTheAge
Copy link
Owner

I will keep this pr open, when I'll add basic support for plugins I'll convert it to a plugin

* Don't use 'weird' output of EncapsulateRect as 'lastRect', but the
  actual rect of the (old) state
* Improve ScrollArea class names to reflect horizontal scrolling as
  well, including a constructor parameter to enable horizontal scrolling
* Renamed some variables
* Typos
…r header/title text

Note that the horizontal scrollbar is not clickable (ctrl+scroll and
keyboard scrolling is working)
Adding a lastContentRect which contains the (correct/out-of-window)
size, fixes the contentSize calculation in BuildGui().

I did not see any glich(es) anymore, and the ScrollArea size is set
correctly now (showing scrollbars if needed)
It is a bit hackish but it seems to work (after updating the UI to
trigger some 'recalculation')

Fixes: #2
have-fun-was-taken added a commit to have-fun-was-taken/yafc-ce that referenced this pull request Feb 13, 2024
It is a collection of changes I made for
ShadowTheAge#145 to fix various things.
In order to make this (big) PR adding the summary view smaller, I moved
this (and some other changed) to their own PR, before submitting the PR
to add the summary view.

List of improvements/fixes:
* _Don't use 'weird' output of `EncapsulateRect()` as 'lastRect', but
the actual rect of the (old) state_
`EncapsulateRect()` changes the returned rectangle while processing it,
causing rendering issues (I forgot the details, it is ages ago)
* _Improve ScrollArea class names to reflect horizontal scrolling as
well, including a constructor parameter to enable horizontal scrolling_
The ScrollArea is capable of having both vertical and horizontal
scrollbars. The horizontal scrollbar is never used, and was not exposed.
But the summary view needs it for larger factories or the user needs an
ultra, mega, super wide screen 😛
* _Renamed some variables_
  For clarification
* Typos and some formatting
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.

None yet

2 participants