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

Maroon-437 Add Network Simulator experiment #560

Open
wants to merge 42 commits into
base: develop
Choose a base branch
from

Conversation

jakobstanta
Copy link
Collaborator

closes #437

@jakobstanta jakobstanta linked an issue Dec 20, 2024 that may be closed by this pull request
@FlorianGlawogger FlorianGlawogger self-requested a review January 15, 2025 10:18
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.

The experiment works very well and is interesting look at. Very well implemented, clean and well-structured code! Awesome work! I just made a few code suggestions, but there shouldn't be much work left, I think mostly, changing namespaces and setting some methods private and such. Again: very well done! 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not related to this file, but you forgot to add the Network Simulator experiment to the ComputerScience category in the Assets/Maroon/GlobalEntities/Management/SceneManager/SceneManager.prefab

Copy link
Collaborator

Choose a reason for hiding this comment

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

The table does not have a proper collider (only the pc screens have a collider)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not related to this file, but unity/Assets/Resources/MLG/laboratory.xml needs to have the "press enter to enter network sim experiment blabla" added

Copy link
Collaborator

Choose a reason for hiding this comment

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

is missing the ScoreView and InfoStand, please look at other laboratoryBlocks from other experiments to see what I mean

Copy link
Collaborator

Choose a reason for hiding this comment

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

if you want to, you could mark the faces on the outside and inside of the tube as "Smooth" shaded (not the flat circle surfaces though);
but you don't have to, just a suggestion^^

Comment on lines +127 to +135
public readonly struct Preset {
public readonly DevicePreset[] Devices;
public readonly (int, int)[] Cables;

public Preset(DevicePreset[] devices, (int, int)[] cables) {
Devices = devices;
Cables = cables;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this could be made a ScriptableObject, but you can also leave it like it is

Comment on lines +160 to +171
ClearNetwork();
var preset = NetworkPresets.Presets[index - 1];
foreach(var device in preset.Devices) {
var instance = Instantiate(devicePrefabs[(int)device.Type], networkArea.transform);
instance.transform.localPosition = device.Position;
instance.PresetInitialize(networkArea);
networkDevices.Add(instance);
}
foreach(var connection in preset.Cables) {
addCableScript.AddCable(networkDevices[connection.Item1], networkDevices[connection.Item2]);
}
UpdateAddressTables();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would maybe create a separate overloaded method LoadPreset(Preset preset) and LoadPreset(int index) then simply calls that method with the preset;

}
),
};
public readonly struct Preset {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about calling this NetworkPreset?


public DialogueManager DialogueManager { get; private set; }

void Start() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could be made private?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hub and Switch look very similar IMO and it might be a bit difficult to distinguish between them; would it be possible to make them a bit more distinguishable from each other?

@FlorianGlawogger FlorianGlawogger changed the title Feature/maroon 437 network simulator Maroon-437 Add Network Simulator experiment Jan 15, 2025
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.

Network Simulator
2 participants