-
Notifications
You must be signed in to change notification settings - Fork 17
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
Maroon-399 Tools UI #409
base: develop
Are you sure you want to change the base?
Maroon-399 Tools UI #409
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see the request changes,
- Use the Experiment Template scene for the CoulombsLawRemake
- Use the UI Prefab for the CoulombsLawRemake otherwise the UI layout is not correct
- The CoulombsLawRemake simulation is not working when playing the simulation
- Toolkit UI overlaps with Dialog View
- Tools like Ruler are not working in the other scenes
- Please move all experiment relevant scripts into the corresponding experiment folder
@@ -0,0 +1,583 @@ | |||
%YAML 1.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this prefab to the experiment folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -1,18 +0,0 @@ | |||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not remove this file. This is used in the Optics experiment scene.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed to ToggleToolUI.cs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be back again and work properly in the optics experiment
@@ -0,0 +1,7 @@ | |||
fileFormatVersion: 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move the ColoumbsLawRemake scene to the ColoumbsLaw experiment folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -0,0 +1,7 @@ | |||
fileFormatVersion: 2 | |||
guid: 2e69c3f8c37d40e49ac178de75fa1cf0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please copy the experiment room template scene and add your changes to this scene. Otherwise, the experiment room setting is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
using System.Collections.Generic; | ||
using UnityEngine; | ||
|
||
namespace Assets.Maroon.reusableGui.Experiment.ToolsUI.Scripts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please change the namespace to Maroon.UI
using UnityEngine; | ||
using UnityEngine.UI; | ||
|
||
namespace Assets.Maroon.reusableGui.Experiment.ToolsUI.Scripts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change the namespace to Maroon.UI
|
||
namespace Assets.Maroon.reusableGui.Experiment.ToolsUI.Scripts | ||
{ | ||
public class PC_ObjectSelectionHandler : MonoBehaviour |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it just for particle selection? if yes maybe use another name
public class PC_ObjectSelectionHandler : MonoBehaviour | ||
{ | ||
[SerializeField] private GameObject inputXVariable; | ||
[SerializeField] private GameObject inputYVariable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about an inputZVariable?
using Maroon.Physics.Electromagnetism; | ||
using UnityEngine; | ||
|
||
public class ParticleManager : MonoBehaviour |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this to the experiment scene folder
using UnityEngine.EventSystems; | ||
using UnityEngine.UI; | ||
|
||
namespace Assets.Maroon.reusableGui.Experiment.ToolsUI.Scripts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change the namespace.
This script should also be moved to the experiment folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Fix issue
35df500
to
cfd5289
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking a great addition and I'm sure lots of new and old experiments will benefit from such a Tools UI 👍 what's IMO the biggest issue right now is that some functionality from Coulomb's Law experiment is missing now (e.g. 3D variant), and made some suggestions how code might be a bit more readable, but feel free to add your own suggestions/feedback to that
unity/Assets/Maroon/scenes/experiments/CoulombsLaw/CoulombsLaw.pc.unity.meta
Outdated
Show resolved
Hide resolved
unity/Assets/Maroon/scenes/experiments/CoulombsLaw/CoulombsLaw.pc.unity
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
German translation not working in the scene
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used anywhere? if not can it be deleted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the name ToolDictionary implies this contains a Dictionary, with a key value storage, but it's just a list, so I'd suggest renaming this class;
unity/Assets/Maroon/scenes/experiments/CoulombsLaw/Scripts/PC_ObjectSelection.cs
Outdated
Show resolved
Hide resolved
unity/Assets/Maroon/scenes/experiments/CoulombsLaw/Scripts/PC_ObjectSelection.cs
Outdated
Show resolved
Hide resolved
unity/Assets/Maroon/scenes/experiments/CoulombsLaw/Scripts/PC_ObjectSelectionHandler.cs
Outdated
Show resolved
Hide resolved
unity/Assets/Maroon/scenes/experiments/CoulombsLaw/Scripts/PC_ObjectSelectionHandler.cs
Outdated
Show resolved
Hide resolved
unity/Assets/Maroon/scenes/experiments/CoulombsLaw/Scripts/ParticleManager.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot better already, though I think there are still some unresolved issues from my last review, and I found a few more things that don't yet work, but otherwise looks already good👍
EDIT: also needs to be merged, conflicts need to be resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, but I can't seem to find out if this file is used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the charges can be forced out of the whiteboard area somehow:
https://github.com/user-attachments/assets/6a185f42-6162-488c-9fe0-ec9aac8b0be5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Measure distance points are also reset when pressing reset button, (not sure if that should be done tbh), and when reset, the line between them does not change:
resetMeasureDistance.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Voltage measure pins are removed when resetting, maybe just remove distance measure things as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outline of selected charges not visible, see #409 (comment)
Hi, please have a look at the position of the files.
closes #399