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 scrollbars to the canvas #11

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

Conversation

sphaero
Copy link
Contributor

@sphaero sphaero commented Aug 28, 2019

This is a fix for #5.

It replaces the canvas's offset vector with the scroll tracking of the window.

@sphaero
Copy link
Contributor Author

sphaero commented Aug 28, 2019

btw, I'm not sure it works with imgui version 1.70. I had some issues so I upgraded to latest but can't recall what it was exactly. I created #12 for the cmake changes

@rokups
Copy link
Owner

rokups commented Aug 29, 2019

Thank you for a great PR! This is the most useful feature. There is one detail. If for example i scroll to the left using mouse - scrollbar shrinks allowing me to go back to that empty space i once viewed by using scrollbar. But that empty space is otherwise not useful. I think it might be confusing to the user giving a false impression that some nodes are located there.

Do you think we could have scollbars providing access to are a which is a combined rectangle of all nodes + rectangle of entire current visible area? This way if we scroll to the either end of the canvas using scrollbar - we would end up looking at last nodes on that end. What do you think?

@sphaero
Copy link
Contributor Author

sphaero commented Aug 29, 2019

Yes indeed. I'm aware of that and I'm still thinking about how to achieve that most easily. I was thinking to use SetCursorPos (which tracks CursorMaxPos) to track the minimal position of what is drawn on the canvas. This could be done in the BeginNode method.

I'll get to that. Either by adding it to this PR or a new one?

@rokups
Copy link
Owner

rokups commented Aug 29, 2019

Maybe you could add something like ImRect combined_rect field to _CanvasStateImpl, then in each EndNode() append rect of the node to combined_rect. Also append rect of entire canvas in EndCanvas() and render scrollbar there. combined_rect should also be cleared in BeginCanvas().

I'll get to that. Either by adding it to this PR or a new one?

Your call. If you figure out a better implementation than current one - feel free to force-push to branch of this PR. Do not forget to rebase on master as well ;)

@sphaero
Copy link
Contributor Author

sphaero commented Aug 29, 2019

I had a go at it, but it still has some issues. Basically all I do in EndNode() is:

canvas->contents_rect.Min = ImMin(canvas->contents_rect.Min, node_pos);
canvas->contents_rect.Max = ImMax(canvas->contents_rect.Max, node_pos + node_rect.GetSize());

In BeginCanvas() I initialize the contents_rect as the visible window rect in canvas coords:

canvas->contents_rect = ImRect(canvas->rect.Min + w->Scroll, canvas->rect.Min + w->Scroll + w->InnerClipRect.GetSize());

The contents_rect also needs to size on edge scrolling.

It can work but I run in the sticky window edges issue which gets worse now. I think it's caused by the scrollbars which need to be drawn or not since it happens only if there is a scrollbar not visible. I think it might also be done with only one rect in _CanvasStateImpl instead of two.

I need to test some more

@rokups
Copy link
Owner

rokups commented Feb 14, 2020

We sort of forgot this PR for a while. What is it's state? Did 60f251c solve remaining issues and is this in mergeable state?

@sphaero
Copy link
Contributor Author

sphaero commented Feb 14, 2020

Don't merge it yet. I still haven't found an approach that doesn't have the side effects. I haven't worked on it either. So either close this PR or leave it open until I have found a better approach.

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