Eh, I like the approach he took with it. I use a lot of this sort of math and it simplifies logic at the cost of somewhat obfuscating code behind math which for any reasonably good programmer shouldn't be too much of a problem.
Yes, I love the modulus operator. I would marry it in a heartbeat. But personal fetishes aside, it is technically less straight forward. A double nested loop is easily readable by anyone, including a novice programmer. A single loop using modulus, as glorious and foundational as it is, takes a bit of experience to understand.
It's also not the best fit for the problem. The DivMod code is useful to map a 1D index into a pair of 2D coordinates, while the reverse, multiplication and addition, can map 2D coordinates into a 1D index. Here you have 2D coordinates that will be mapped over a 2D range. Just stick with 2D.
I'm not sure of the performance gains/penalties with either approach... profiling would help here but at least for me the lack of a second loop makes perfect sense.
Performance wise it doesn't matter much since you will only call the function once or twice at initialization to set lava possible areas.
Correct, performance doesn't really matter here. You'd likely need a very large number of operations to measure any difference.
But, when there are details, I can never resist diving into them. In this case, the DivMod code is likely to be a bit slower. The modulus (%) and division (/) operations are relatively slow compared to other integer operations, such as increment (++). Further, the DivMod is done for every iteration of the inner loop, while in a doubly nested loop, the outer loop's increment is only done once for each full iteration of the inner loop.
One additional point to note is that at the assembly level, a single instruction (DIV) produces both the result for division (in EAX) and modulus (in EDX). The code as written may execute the same sequence of instructions twice, or an optimizing compiler could pick this up and reduce it down to a single operation and harvest both outputs (EAX and EDX registers) at the same time. Useful, since this instruction is the slow one.
But again, performance just isn't going to be an issue here. Another caveat, the above was theory, not profiling data.
It is a little odd that both approaches are used right next to each other.
Yes.
If we remove the namespace from the code, will it be a problem if other custom scenarios have already defined their own function with the same name and reference OP2Helper?
Yes. Though if there are name collisions here, it's likely duplicate code, or a highly compatible alternate implementation. In which case the solution is to remove the duplicate code, which should probably be done anyway.
EDIT 2: This function needs to ensure that x1 and y1 are less then x2 and y2 unless MAP_RECT automatically does that.
The DivMod implementation has a similar issue. The numberOfTiles value could be negative. I don't think this is worth worrying about. It's generally understood the first coordinate is lower valued.
I would propose that we wrap the entire SDK into a namespace. It can be a bit painful as my experience doing that with NAS2D demonstrated but I think it will produce a cleaner output and will avoid name collisions as you mentioned. But I think that's a topic for a different discussion.
I agree, and would like to see things in a namespace too. Though I believe there was one showstopper on that idea. Namespaces affect mangled names, which means putting any imports from Outpost2.exe into a namespace will lead to link errors. In case I'm wrong here, try wrapping some Outpost2DLL code in a namespace and see what happens when you compile.
Granted, the namespacing is more of an issue for Outpost2DLL with direct imports from Outpost2.exe, as opposed to OP2Helper, which uses only indirect imports through Outpost2DLL. Guess I hadn't considered that before. Perhaps we should consider namespaces for OP2Helper and other projects outside of the Outpost2DLL project.
