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

Netutil cleanup #1273

Merged
merged 9 commits into from
Jan 7, 2022
Merged

Netutil cleanup #1273

merged 9 commits into from
Jan 7, 2022

Conversation

kianzarrin
Copy link
Collaborator

@kianzarrin kianzarrin commented Jan 4, 2022 •
edited
Loading

  • moved network related utilities to extensions.
  • moved iterators to iterator subfolder
  • introduced LaneUtil.
  • I have marked some methods as obsolete instead of removing them to avoid big changes.

kianzarrin
kianzarrin commented Jan 4, 2022
View reviewed changes
TLM/TLM/UI/MainMenu/DebugMenu.cs Outdated
@@ -191,9 +192,10 @@ public class DebugMenuPanel : UIPanel
private void ClickGoToNode(UIComponent component, UIMouseEventParameter eventParam) {
ushort nodeId = Convert.ToUInt16(_goToField.text);

if ((nodeId.ToNode().m_flags & NetNode.Flags.Created) != NetNode.Flags.None) {
CSUtil.CameraControl.CameraController.Instance.GoToNode(nodeId);
if ((nodeId.ToNode().m_flags & NetNode.Flags.Created) == NetNode.Flags.None) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unintended change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is result of auto-fix.
at least the functionality has not change!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok reverted it.

originalfoo
originalfoo reviewed Jan 5, 2022
View reviewed changes
TLM/TLM/Util/Extensions/NetNodeExtensions.cs Outdated Show resolved Hide resolved
TLM/TLM/Util/Extensions/NetSegmentExtensions.cs Outdated Show resolved Hide resolved
TLM/TLM/Util/Extensions/NetSegmentExtensions.cs Outdated Show resolved Hide resolved
TLM/TLM/Util/Extensions/NetSegmentExtensions.cs Outdated Show resolved Hide resolved
TLM/TLM/Util/Extensions/NetSegmentExtensions.cs Show resolved Hide resolved
TLM/TLM/Util/Extensions/NetSegmentExtensions.cs Show resolved Hide resolved
TLM/TLM/Util/Extensions/NetSegmentExtensions.cs Show resolved Hide resolved
TLM/TLM/Util/Extensions/NetSegmentExtensions.cs Show resolved Hide resolved
TLM/TLM/Util/Extensions/NetSegmentExtensions.cs Show resolved Hide resolved
TLM/TLM/Util/Extensions/NetSegmentExtensions.cs Show resolved Hide resolved
@kianzarrin
Copy link
Collaborator Author

@aubergine10 note that I have merely moved the code. I have not written any new code. So I prefer not to change things too much. Maybe just IsFlagSet and LHT ?

@originalfoo
Copy link
Member

@aubergine10 note that I have merely moved the code. I have not written any new code. So I prefer not to change things too much. Maybe just IsFlagSet and LHT ?

Yup, the other stuff can come later. Want me to create issues to track that, or...?

@originalfoo originalfoo mentioned this pull request Jan 5, 2022
Closed
@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Jan 5, 2022 •
edited
Loading

@aubergine10 note that I have merely moved the code. I have not written any new code. So I prefer not to change things too much. Maybe just IsFlagSet and LHT ?

Yup, the other stuff can come later. Want me to create issues to track that, or...?

Do you think we need an issue for ternary condition instead of if/else ? Some coding standards ban using ternary if as it can cause confusion. I think for the sake of TMPE either should be acceptable. More over any change of code that has been thoroughly tested for years can introduce new points of failure.

as for the magic number, and some other stuff you can open an issue.

BTW if this is performance critical then we have a problem with producing too much garbage and using slow code.

@kianzarrin
Copy link
Collaborator Author

maybe when you opened an issue I can reference it in the TODO over there.

@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Jan 5, 2022 •
edited
Loading

@aubergine10 I made a few changes but please mark everything inside GetSortedLanes() as resolved

@kianzarrin kianzarrin self-assigned this Jan 5, 2022
@kianzarrin kianzarrin added the code cleanup Refactor code, remove old code, improve maintainability label Jan 5, 2022
kvakvs
kvakvs approved these changes Jan 6, 2022
View reviewed changes
Copy link
Collaborator

@kvakvs kvakvs left a comment

Choose a reason for hiding this comment

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

i like this

@kianzarrin
Copy link
Collaborator Author

@aubergine10 I made a few changes but please mark everything inside GetSortedLanes() as resolved

I hope you didn't forget about this

originalfoo
originalfoo reviewed Jan 6, 2022
View reviewed changes
TLM/TLM/Util/LaneUtil.cs Outdated
if (curLaneId == laneId) {
return laneIndex;
}
laneIndex++;
Copy link
Member

Choose a reason for hiding this comment

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

Did you commit to wrong branch?

a4d5a79

Copy link
Member

Choose a reason for hiding this comment

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

Yep it's a bug here

Copy link
Member

Choose a reason for hiding this comment

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

Oh... ook @aubergine10 #1275 is targeting this branch, so... confusing but fixed. We just need to merge #1275 first, I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes that is a bug that I already fix. I am surprised the fix was not uploaded.

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 had committed the fix to the wrong branch. now is fine.

krzychu124
krzychu124 requested changes Jan 6, 2022
View reviewed changes
Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

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

See comment

krzychu124
krzychu124 approved these changes Jan 7, 2022
View reviewed changes
Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

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

👍

@kianzarrin kianzarrin merged commit fe93f68 into master Jan 7, 2022
@kianzarrin kianzarrin deleted the netutil-cleanup1 branch January 7, 2022 17:34
@krzychu124 krzychu124 mentioned this pull request Jan 8, 2022
Merged
@originalfoo originalfoo modified the milestones: 11.6.0, 11.6.2 Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@originalfoo originalfoo originalfoo approved these changes

@kvakvs kvakvs kvakvs approved these changes

@krzychu124 krzychu124 krzychu124 approved these changes

@DaEgi01 DaEgi01 Awaiting requested review from DaEgi01

Assignees

@kianzarrin kianzarrin

Labels
code cleanup Refactor code, remove old code, improve maintainability
Projects
None yet
Milestone
11.6.2
Development

Successfully merging this pull request may close these issues.
4 participants
@kianzarrin @originalfoo @kvakvs @krzychu124