Code tidying 1 (my first pull request ever)

[AlonzoTG]

hello, trying to figure out this github thing everyone's hyper about.

I actually got it to build and threw some tools at it. This build works VERY well, tested a number of game activities against it and all good except for an auto-pilot bug related to cargo ships, see theory regarding that in first commit. Obviously, it is not release-able with that problem but I don't know enough C# to fix it.

I was building a 30,000 part "world ship" with all 3 symmetry planes enabled and making edits to it with this version was dramatically faster than with stock.

My ambition is to port this to a Vulkan back-end to pave the way for Linux and cross-platform deployment.

[Evrey]

You are using version control. Why are you polluting the code with commented-out-lines? Why are you removing var?

But most interestingly...

Why are you removing all those generic getters, replacing them with functions taking type objects?

Besides that... thanks for tidying up parts of the code base.

[AlonzoTG]

1. commented out lines: I see it as a stage in removal, If I'm not SURE something should be gone AND it would be difficult to put back if it isn't, then I just comment it out. I did actually remove several files worth of commented lines so I'm definitely ahead of the game with regards to comment pollution.
2. Var is a missfeature, it should only be used when you are writing a generic function or, as in C++, the type specification is just too unwieldy. In this case what you were doing was trying to pick cherries out of a barrel of apples. You would take some collection and then grab a type you want, such as the attribute mechanism. I think I was using a different version of C#, it didn't like how you were querying attributes. I had to fix that. Now when it went to use the newly-extracted variable, it couldn't figure out the type so either I had to cast it in a dozen places or simply make it a specified type. -- so I made it a specified type.

[mze9412]

This won't be merged for sure, sorry.

1) var has nothing to do with generic functions. It is not dynamically typed but still a fixed type, it just saves you from writing it by hand and makes refactoring simpler. Don't just remove it because you can.

2) If you comment out stuff and don't know if it can be commented out then do not comment it out. Either you know or you don't, before you create a PR. Please :)

3) What did you not like about GetCustomAttribute<>()? Why did you change that everywhere to the more generic method GetCustomAttribute(Type attributeType)?

I don't really get many of the changes. Some I get, but many seem senseless. Especially the GetCustomAttribute and var changes. Its just replacing one line with another line which exactly does the same with no benefits or detriments for using either of them. confused

[Jimmacle]

Please don't just throw tools at code, especially when you end up breaking things you admit to not knowing how to fix. There are also coding standards on the repository's wiki that should be followed. You also say it's "dramatically faster," so can we see some profiling/benchmarks?

[AlonzoTG]

Well, thank you for your feedback. My tools told me that such lines could be commented out however I know that my tools cannot check against the intended program logic. I am aware of this, hence what I did.

On that first commit, the toolchain I was using really was throwing errors at me. =\ After that, I was balancing a single variable specification with a dozen casts, I did not text-search "var" and grab a cup of coffee...

[mze9412]

I just saw that you modified a lot if .csproj project files and also modified the solution. That switched the whole thing from .net 4.5 to .net 4.6.1 and is something that you absolutely should not do. Ever. If KSH wants that, let them do it, don't just move their code over (I believe internally they already did that).

Examples:

-<Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
 +<Project ToolsVersion="4.6" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

-    <TargetFrameworkVersion>v4.0</TargetFrameworkVersion>
 +    <TargetFrameworkVersion>v4.6.1</TargetFrameworkVersion>

-<Project ToolsVersion="4.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
 +<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

[M-Ro]

Sources/Sandbox.Game/Engine/Voxels/MyMarchingCubesMesher.cs

- if (Math.Abs(MyVoxelConstants.VOXEL_ISO_LEVEL - contentA) < 0.00001f)
+ if (Math.Abs(MyVoxelConstants.VOXEL_ISO_LEVEL - contentA) == 0)

Changes like this will almost certainly introduce more issues. There is a reason a threshold value was used there rather than 0.

You have also removed several files related to the RakNet network layer, which I believe was planned to be used with the Xbox port. (I may be wrong here)

It's been said already, but please don't just throw tools at code. Many of these changes were needless and/or dubious.

[AlonzoTG]

I am working on a revised version of this request but let's keep the conversation going.

If you squint at the actual type there, you will see that it is actually an integer, so therefore it was a bug because it was doing a floating point comparison on an integer! There are also many examples of the reverse, only some of which I fixed.

[Jimmacle]

I wouldn't really call it a bug. When you're making changes you have to gauge whether the cost of someone having to test/review it is worth the improvement. Changes like removing usings don't benefit the code at all, the same with changing var and generics (I read through your changes and the removal of those really hurt readability). However, small/localized PRs with proven benefits to the code are much more likely to get merged.