View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0000224 | Spring engine | General | public | 2006-07-01 22:47 | 2006-08-05 00:17 |
| Reporter | Sean Mirrsen | Assigned To | |||
| Priority | normal | Severity | feature | Reproducibility | always |
| Status | closed | Resolution | fixed | ||
| Summary | 0000224: [patch] Rotating Buildings | ||||
| Description | This allows to orient the buildings you place before construction in 90 degree increments. It still might have various quirks, such as AI incompatibility or net desync (couldn't test), and the code is somewhat bulky at places, but it works fine. Also, I couldn't find how to make the patch have relative filenames instead of absolute, so another patch will occur if this one's unusable. | ||||
| Additional Information | You have to add "bind '<' spinbuilddir" and "bind '>' unspinbuilddir" to uikeys.txt for the feature to work. | ||||
| Tags | No tags attached. | ||||
| Attached Files | |||||
| Checked infolog.txt for Errors | |||||
|
|
Ok, I finally found time to review it properly now my last exam is finally done. First off, there's too much code duplications, as Jelmer said earlier. Mostly these switch statements. You should see a common thing in all of them: they read xsize,ysize from a unitdef and swap them around a bit based on one integer variable. So I suggest you add 2 functions to CUnitDef (you can leave comments out), look at dontLand in CUnitDef for an example. int GetXsize(int facing) const { // If facing is even, xsize is returned. // If facing is odd, ysize is returned. return (facing & 1) == 0 ? xsize : ysize; } int GetYsize(int facing) const { // If facing is even, ysize is returned. // If facing is odd, xsize is returned. return (facing & 1) == 1 ? xsize : ysize; } Having done this you can replace all switch statements by code like (notice the massive code reduction this gives!): int temp_xsize = ud->GetXsize(facing); int temp_ysize = ud->GetYsize(facing); For the yardmap I noticed you introduced a yardmapLevel as an array of yardmaps. Using that could save you from the switch statement for choosing the right yardmap. If you commented it out because you didn't get it working, you can do the yardmap the same as xsize and ysize above: make a function 'unsigned char* GetYardmap(int facing) const' and replace the switch statement by a call to that function. Also, it seems you implemented it using rxsize and rysize, but I didn't find any assignments to those other then rxsize = xsize and rysize = ysize, suggesting rxsize and rysize are just redundant. If this is true, could you remove rxsize and rysize and substitute references to them with xsize and ysize. Almost the same applies to the yardmap. I noticed that after applying the patch, there are 5 yardmaps, one for every direction and one extra for direction 0. I'd suggest you remove your yardmap1 and use yardmap instead, as they (yardmap and yardmap1) seem to have equal content. This way the API will be backwards compatible with AIs. One last point: you still use if (c.params[3]) facing = c.params[3]; This is invalid and may crash/desync/etc, because you reference element 3 in the if, when it is possibly not available. What you want here is: if (c.params.size() >= 4) facing = c.params[3]; I hope I'm not demotivating you by giving so much comments so late after you submitted it. I rather hope you learn from it, so your next patches will be right the first time :-) cheers Tobi |
|
|
I didn't introduce YardmapLevels, some of the original devs did. I don't have the slightest idea of what it's for. rxsize and rysize are used as storage for Real sizes, as a reference to set the xsize and ysize when a non-square building is rotated. I left the original yardmap to avoid switching it around, and just setting it to one of the four other yardmaps as per the facing. As for the code reduction with those processes, I think a reference to a set variable is less cpu-intensive than calling a procedure, no matter how microscopic the difference might be. Sure it looks prettier in code form, but the compiled result seems better done my way. I'll have a go at doing it your way though, it might just work. ;) I need assistance rotating building decals, working out a formula to rotate the vertexarray in creation is boiling my brain... |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2006-07-01 22:47 | Sean Mirrsen | New Issue | |
| 2006-07-01 22:47 | Sean Mirrsen | File Added: Rotating Buildings.diff | |
| 2006-07-06 17:14 | tvo | Note Added: 0000281 | |
| 2006-07-06 18:51 | Sean Mirrsen | Note Added: 0000284 | |
| 2006-08-05 00:17 | jcnossen | Status | new => closed |
| 2006-08-05 00:17 | jcnossen | Resolution | open => fixed |
