-
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-437 Add Network Simulator experiment #560
base: develop
Are you sure you want to change the base?
Conversation
…ts travelling through cables
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 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! 😄
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.
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
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 table does not have a proper collider (only the pc screens have a collider)
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.
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
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 missing the ScoreView and InfoStand, please look at other laboratoryBlocks from other experiments to see what I mean
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.
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^^
public readonly struct Preset { | ||
public readonly DevicePreset[] Devices; | ||
public readonly (int, int)[] Cables; | ||
|
||
public Preset(DevicePreset[] devices, (int, int)[] cables) { | ||
Devices = devices; | ||
Cables = cables; | ||
} | ||
} |
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 could be made a ScriptableObject, but you can also leave it like it is
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(); |
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.
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 { |
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 do you think about calling this NetworkPreset
?
|
||
public DialogueManager DialogueManager { get; private set; } | ||
|
||
void Start() { |
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.
could be made private?
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.
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?
closes #437