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

Toggle crosshair and perspective background #1153

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from
Open

Conversation

fsdavid
Copy link
Contributor

@fsdavid fsdavid commented Feb 3, 2022

No description provided.

@xgui3783
Copy link
Member

xgui3783 commented Feb 3, 2022

Thanks for the new PR @fsdavid .

Do you want me to re add the review from last time?

Also, somewhat related, can you go through https://github.com/FZJ-INM1-BDA/siibra-explorer/branches and delete the branches you don't need?

@fsdavid
Copy link
Contributor Author

fsdavid commented Feb 3, 2022

Sure, I will clean the branches. I will use review from here #1148

Copy link
Member

@xgui3783 xgui3783 left a comment

Choose a reason for hiding this comment

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

In addition, can you also please add typing to the methods you introduced?

edit: and also change the PR name to something that makes sense?

src/ui/config/configCmp/config.component.ts Outdated Show resolved Hide resolved
src/ui/config/configCmp/config.component.ts Outdated Show resolved Hide resolved
src/ui/config/configCmp/config.component.ts Outdated Show resolved Hide resolved
src/ui/config/configCmp/config.component.ts Outdated Show resolved Hide resolved
Comment on lines 253 to 256
public set showBackground(value) {
this.nehubaViewer.config.layout.useNehubaPerspective.drawSubstrates.color[3] = value ? 0.2 : 0
this.nehubaViewer.redraw()
}
Copy link
Member

Choose a reason for hiding this comment

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

You can set .drawSubstrates to falsy, I think.

It should achieve the same result, perhaps more performant (since gl will not even need to try to draw the substrate)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting it simply true/false is changing the default opacity of the background

@fsdavid fsdavid changed the title Dev view settings Toggle crosshair and perspective background Feb 11, 2022
@fsdavid
Copy link
Contributor Author

fsdavid commented Feb 13, 2022

@xgui3783 Should be fixed now

Copy link
Member

@xgui3783 xgui3783 left a comment

Choose a reason for hiding this comment

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

Codewise, Just some minor changes.

Thanks for the good work.

src/ui/config/configCmp/config.component.ts Outdated Show resolved Hide resolved
src/ui/config/configCmp/config.component.ts Outdated Show resolved Hide resolved
src/ui/config/configCmp/config.component.ts Outdated Show resolved Hide resolved
src/ui/config/configCmp/config.component.ts Outdated Show resolved Hide resolved
src/ui/config/configCmp/config.component.ts Outdated Show resolved Hide resolved
src/ui/config/configCmp/config.component.ts Outdated Show resolved Hide resolved
src/ui/config/configCmp/config.component.ts Outdated Show resolved Hide resolved
@xgui3783
Copy link
Member

also, it appears that the background colour changer is not working properly.

at start, it sets to black

and on update, it will always reset to white.

Can you add some tests to ensure the methods works as intended?

I have a feeling that we want this in the gear icon mat menu, rather than in config. It's rather hard to find.

@fsdavid
Copy link
Contributor Author

fsdavid commented Feb 15, 2022

I have a feeling that we want this in the gear icon mat menu, rather than in config. It's rather hard to find.

To avoid overloading of the perspective views gear icon, which more feels a quick setting of the perspective view than all 4 views, I think using the Settings window is a good way. I also would like if users will get used to using the settings window, as it could hold more settings.

@xgui3783
Copy link
Member

I have a feeling that we want this in the gear icon mat menu, rather than in config. It's rather hard to find.

To avoid overloading of the perspective views gear icon, which more feels a quick setting of the perspective view than all 4 views, I think using the Settings window is a good way. I also would like if users will get used to using the settings window, as it could hold more settings.

I agree that perspective setting menu can become quite overloaded.

At the same time, I also think at least the substrate colour can be configured there, since it is a more natural place to try to find the setting.

We can duplicate the UI in user setting menu (where it is currently), as well

@xgui3783
Copy link
Member

also, I found a bug, steps to reproduce:

1/ load jba 2.9,
2/ go to setting, turn off axis, background, close setting dialog (noting that axis and substrate disappears, as expected)
3/ go to setting again, axis/background checkbox are both checked.

I would argue use a dedicated service, or ngRx/componentStore which can be used to persist component state.

@@ -38,9 +28,9 @@ export class ConfigStore extends ComponentStore<ConfigState> {
togglePerspectiveViewSubstrate: false
})

const nehubaInterval = setInterval(() => {
// ToDo find better way to get nehubaviewer containing cofig
setTimeout(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xgui3783 on my local machine viewerCtrlCmp.component.ts > ViewerCtrlCmp > toggleParcVsbl > if _flagDelin is false > calls schedulRedraw FAILED test is failing? (it was not failed here, but I think potentially it could) could it be caused by setTimeout? if it is, would you have any idea how to get nehubaviewer containing config on to set initial values?

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.

Possibility to hide crosshair in orthogonal views and bckground colouring of section planes in 3D view
2 participants