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.

Sign up for GitHub

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

Jump to bottom

Maroon-506 Load config from URL #515

Merged
merged 67 commits into from
Nov 27, 2024
Merged

Maroon-506 Load config from URL #515

merged 67 commits into from
Nov 27, 2024

Conversation

Averimon
Copy link
Collaborator

@Averimon Averimon commented Oct 1, 2024

Closes #506

@Averimon Averimon self-assigned this Oct 1, 2024
@Averimon Averimon linked an issue Oct 1, 2024 that may be closed by this pull request
Load config from url #506
Closed
FlorianGlawogger
FlorianGlawogger requested changes Oct 9, 2024
View reviewed changes
Copy link
Collaborator

@FlorianGlawogger FlorianGlawogger left a comment

Choose a reason for hiding this comment

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

Had a few minor remarks, otherwise my biggest concern is that I am not entirely sure what the issue #506 refers to (see comments), but generally works well, and very clean and nice code IMO 👍

unity/Assets/Maroon/GlobalEntities/WebGLUrlParameterReader/Scripts/WebGLUrlParameterReader.cs Show resolved Hide resolved
unity/Assets/Maroon/GlobalEntities/StreamingAssetsLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/GlobalEntities/StreamingAssetsLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/reusablePrefabs/experimentSetting/ExperimentSetting.pc.prefab Outdated Show resolved Hide resolved
unity/Assets/Maroon/reusableScripts/Config/Maroon.Config.asmdef Outdated Show resolved Hide resolved
...ssets/Maroon/scenes/experiments/3DMotionSimulation/Scripts/ConfigLoader3DMotionSimulation.cs Outdated Show resolved Hide resolved
...aroon/scenes/experiments/3DMotionSimulation/Scripts/ParameterController3DMotionSimulation.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/GlobalEntities/WebGLUrlParameterReader/Scripts/WebGLUrlParameterReader.cs Show resolved Hide resolved
unity/Assets/Maroon/GlobalEntities/WebGLUrlParameterReader/Scripts/WebGLUrlParameterReader.cs Show resolved Hide resolved
unity/Assets/Maroon/GlobalEntities/WebGLUrlParameterReader/Scripts/WebGLUrlParameterReader.cs Outdated Show resolved Hide resolved
FlorianGlawogger
FlorianGlawogger requested changes Oct 30, 2024
View reviewed changes
Copy link
Collaborator

@FlorianGlawogger FlorianGlawogger left a comment

Choose a reason for hiding this comment

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

Very nice changes. works fine and code is generally very well structured 👍 Most of the comments are some nitpicks, however, i think the main issue is merging with develop branch, and thus generalizing the parameterloader/config for more than one experiment (e.g. 3D motion and Optics), as the develop branch contains the new optics changes, which resulted in the generalized ExperimentParameters and so on?

unity/Assets/Vendor/SimpleJSON.meta Outdated Show resolved Hide resolved
unity/Assets/Maroon/GlobalEntities/StreamingAssetsLoader/StreamingAssetsLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/GlobalEntities/StreamingAssetsLoader/StreamingAssetsLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/scenes/experiments/3DMotionSimulation/Scripts/ParameterLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/reusableScripts/Config/ConfigLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/scenes/experiments/3DMotionSimulation/Scripts/ParameterLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/scenes/experiments/3DMotionSimulation/Scripts/ParameterLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/scenes/experiments/3DMotionSimulation/Scripts/ParameterLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/scenes/experiments/3DMotionSimulation/Scripts/ParameterUI.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/scenes/experiments/3DMotionSimulation/Scripts/ParameterUI.cs Outdated

private void OnConfigLoaded()
{
dropdown.SetValueWithoutNotify(ConfigLoader.Instance.CurrentConfigIndex);
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO it is a bit confusing that ParameterUI has an eventlistener for OnConfigLoaded where it just sets the dropdown to the new config; and the ParameterLoader also has an eventlistener for OnConfigLoaded, where it calls the LoadParameters method of the ParameterUI again.

And if the user changes the chosen preset via the dropdown, then the dropdown calls the DropdownListener of the ParameterUI, which calls ChangeConfig of the ConfigLoader, which invokes the OnConfigLoaded of the ParameterUI (which does nothing in that case) and the OnConfigLoaded of the ParameterLoader, which then calls the LoadParameters function of the ParameterUI again... So this is very confusing and a bit hard to comprehend.

maybe we can get rid of one of the classes or the parametersUI class is only responsible for UI things? But see the main comment of the review, as merging with develop will probably lead to some sort of refactoring anyways?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will leave it for now, but I'll keep it in mind for the refactoring later!

FlorianGlawogger
FlorianGlawogger reviewed Oct 31, 2024
View reviewed changes
unity/Assets/WebGLTemplates/MaroonWeb/configs.php
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a quick thought: are we sure Peter's server supports/allows php? (If not would something like described here https://stackoverflow.com/a/4534000 work?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have already texted him, and it sounds like his server supports PHP, but I will wait for a clearer answer on his side.

@Averimon
Added general url params loading
33bb864
@Averimon
Finished url parameter loading
b3f475a
@Averimon
Added abstract ConfigLoader
7e0ca6d
@Averimon
Added ConfigLoader for 3DMotionSimulation
d7c3146
@Averimon
Moved configs to StreamingAssets folder
73bbf52
@Averimon
Changed Custom ConfigLoader name
5e0386a
@Averimon
Fixed ConfigLoader not found issue
e8d0cc3
@Averimon
Deprecated old GetUrlParameter function
6b90778
@Averimon
Added StreamingAssetsLoader
b6307fc
@Averimon
Added StreamingAssetsLoader
5ad05f6
@Averimon
Implemented ParameterController3DMotionSimulation
7bc35bf
@Averimon
Changed translations keys for experiment config
d49c11f
@Averimon
Implemented JSON Loading from StreamingAssets folder
d9e9846
@Averimon
Bugfix where default config would not get loaded
4096afc
@Averimon
Fixed PC and VR build not working
afa8579
@Averimon
Removed deprecated comment
f7d4f06
@Averimon
Addedd Obsolete Attribute
f4fd187
@Averimon
Put SAL into subfolder and adjusted assembly def. file
bcb89f7
@Averimon
Removed ConfigLoader3DMotionSimulation from Experiment prefab
fb67dd9
@Averimon
Changed ConfigLoader to general usage
175eab0
@Averimon
Removed accidentally added experiment
1c35ff4
@Averimon
Fixed dropdown options being rewritten
c6023ae
@Averimon
Changed ConfigLoader to dynamically evaluate baseDomain
7abc81f
@Averimon
Fixed UrlParameters not found
9b72380
@Averimon
Squashed commit of the following:
b1cb80d 
commit f301538
Author: Florian <[email protected]>
Date:   Wed Nov 6 09:34:20 2024 +0100

    Maroon-544 Update ReadMe for new experiments (#545)

commit 4f178cf
Author: l_owFi <[email protected]>
Date:   Wed Nov 6 09:30:35 2024 +0100

    Maroon-424 [PlanetarySystem] Add planetary system experiment (#442)

commit 2920487
Author: Florian Glawogger <[email protected]>
Date:   Wed Oct 30 09:17:18 2024 +0100

    Maroon-512 [Optics] Optics configurations now use JSON and support being changed via Javascript (#516)

commit a483a75
Author: Daniel <[email protected]>
Date:   Wed Oct 23 17:08:07 2024 +0200

    Maroon-404 [FallingBallViscosimeter] Add Falling Ball Viscosimeter experiment (#511)

commit 34785ee
Author: Florian Glawogger <[email protected]>
Date:   Wed Oct 23 10:52:16 2024 +0200

    Maroon-533 [HuygensPrinciple] Fix missing references in VR water (#534)

commit d47c6e6
Author: Florian Glawogger <[email protected]>
Date:   Fri Oct 18 11:27:55 2024 +0200

    Maroon-357 Use StandaloneWindows64 as PC/VR build target (#529)

commit 5960606
Author: Florian Glawogger <[email protected]>
Date:   Thu Oct 17 16:57:41 2024 +0200

    Maroon-337 Rename "Build" to "Maroon Build" (#522)

commit 4f66225
Author: Florian Glawogger <[email protected]>
Date:   Thu Oct 17 16:55:43 2024 +0200

    Maroon-386 Improve clock synchronization (#519)

commit df59d84
Author: Florian Glawogger <[email protected]>
Date:   Thu Oct 17 16:28:18 2024 +0200

    Maroon-517 Make DontDestroyOnLoad GlobalEntities root GameObjects beforehand (#518)

commit c34d1bf
Author: Michael Stefan Holly <[email protected]>
Date:   Thu Oct 17 16:18:23 2024 +0200

    Maroon-524 [VandeGraaff] Fix Quantity Min Max Ranges for Voltage and Charge Values (#530)

commit 24939c1
Author: Florian Glawogger <[email protected]>
Date:   Thu Oct 17 16:16:54 2024 +0200

    Maroon-525 Unit test that each experiment has an Assembly Definition (#526)

commit c81059b
Author: Florian Glawogger <[email protected]>
Date:   Thu Oct 17 13:51:24 2024 +0200

    Maroon-527 Unit Test that experiment scripts have a namespace (#528)

commit 3759dd2
Author: mfbrantner <[email protected]>
Date:   Wed Oct 16 09:38:11 2024 +0200

    Maroon-513 Fix CI failure caused by parallel license activation (#514)

commit f79156c
Author: Jonathan Platzer <[email protected]>
Date:   Wed Sep 18 09:52:17 2024 +0200

    Maroon-508 [CathodeRayTube] Port CRT experiment to reusable motion solver (#509)

commit 9cb7da2
Author: florianmarcher <[email protected]>
Date:   Wed Sep 11 13:42:31 2024 +0200

    Maroon-396 [PerlinNoise] Add Perlin Noise experiment (#507)

commit df584f5
Author: Florian <[email protected]>
Date:   Mon Sep 9 15:22:21 2024 +0200

    Maroon-504 Update NCalc package (#505)

commit 29b9bfe
Author: Florian <[email protected]>
Date:   Wed Jul 31 11:50:48 2024 +0200

    Maroon-502 [3DMotion] Support Pi as possible parameter (#503)

commit 450e63a
Author: mfbrantner <[email protected]>
Date:   Wed Jul 31 11:31:47 2024 +0200

    Maroon-499 Add serialize-workflow-action (#500)
@Averimon
Added some UNITY_WEBGL checks
770d035
@Averimon Averimon force-pushed the 506-load-config-from-url branch from 5d35148 to 770d035 Compare November 11, 2024 14:47
FlorianGlawogger
FlorianGlawogger requested changes Nov 13, 2024
View reviewed changes
Copy link
Collaborator

@FlorianGlawogger FlorianGlawogger left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for resolving the issues; I think the only things missing now are some merge conflicts in .meta files, and that we have 2 ParameterLoaders; Sorry about all the back and forth 😅 but after that should be finally ready to be merged 👍

unity/Assets/Maroon/reusableGui/Experiment/Scripts/Runtime/LocalizedSimpleTooltip.cs.meta Outdated Show resolved Hide resolved
unity/Assets/Maroon/reusablePrefabs/ConfigLoader.meta Outdated Show resolved Hide resolved
unity/Assets/Maroon/GlobalEntities/StreamingAssetsLoader.meta Outdated Show resolved Hide resolved
@Averimon
Copy link
Collaborator Author

Looks good, thanks for resolving the issues; I think the only things missing now are some merge conflicts in .meta files, and that we have 2 ParameterLoaders; Sorry about all the back and forth 😅 but after that should be finally ready to be merged 👍

Haha, no worries. I highly appreciate your detailed and thorough checks 😊

FlorianGlawogger
FlorianGlawogger requested changes Nov 20, 2024
View reviewed changes
unity/Assets/Maroon/reusableScripts/Config/JsonFileLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/scenes/experiments/3DMotionSimulation/3DMotionSimulation.pc.unity Outdated Show resolved Hide resolved
unity/Assets/Maroon/reusableScripts/Config/JsonFileLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/reusableScripts/ExperimentParameters/ParameterLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/reusableScripts/ExperimentParameters/ParameterLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/reusableScripts/ExperimentParameters/ParameterLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/reusableScripts/ExperimentParameters/ParameterLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/reusableScripts/Config/JsonFileLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/reusableScripts/Config/JsonFileLoader.cs Outdated Show resolved Hide resolved
unity/Assets/Maroon/reusableScripts/Config/JsonFileLoader.cs Outdated Show resolved Hide resolved
FlorianGlawogger
FlorianGlawogger approved these changes Nov 27, 2024
View reviewed changes
Copy link
Collaborator

@FlorianGlawogger FlorianGlawogger left a comment

Choose a reason for hiding this comment

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

Still works well and the code looks very well structured. Ready to merge now! Nice work, well done! 👍

@FlorianGlawogger FlorianGlawogger merged commit a5b2f75 into develop Nov 27, 2024
8 checks passed
@FlorianGlawogger FlorianGlawogger deleted the 506-load-config-from-url branch November 27, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FlorianGlawogger FlorianGlawogger FlorianGlawogger approved these changes

Assignees

@Averimon Averimon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Load config from url
2 participants
@Averimon @FlorianGlawogger