Viewing Issue Simple Details Jump to Notes ] Wiki ] View Advanced ] Issue History ] Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0006930 [DCSS] Patches minor N/A 2013-04-18 06:00 2013-04-24 02:06
Reporter infiniplex View Status public  
Assigned To mumra
Priority normal Resolution done  
Status resolved   Product Branch 0.13 ancient branch
Summary 0006930: Converted parts of layout_pools to C++
Description Patch features:
1. Converts add_pools, widen_path, and remove_isolated_glyphs LUA functions from layout_pools.des to C++ functions callable from LUA.
2. Adds replace_closest C++-from-LUA function
3. Moves layout_pools to layouts.des and renames it layout_cave_pools.
4. Renames layout_pools.des to layout_gehenna.des because it now just contains (some of) the gehenna layouts
5. Adds a new layout (layout_honeycomb) to layout.des based in the add_pools function
6. Adds a few comments to layout.des
Additional Information
Tags No tags attached.
Attached Files ? file icon 0001-Translated-pools-layout-code-to-C.patch [^] (82,124 bytes) 2013-04-18 06:00 [Show Content]
png file icon layout_honeycomb.png [^] (8,459 bytes) 2013-04-18 06:00

? file icon 0001-Translated-pools-layout-code-to-C2.patch [^] (45,052 bytes) 2013-04-23 05:20 [Show Content]

- Relationships

-  Notes
mumra (developer)
2013-04-18 12:08

layout_honeycomb looks very cool (although it's very hard to see in those tiny images, for instance I couldn't see the doors at all without zooming in loads!)

The patch seems to change a lot of stuff - it might have been slightly easier to see what was going on in 3 or 4 separate patches rather than so much stuff squashed into one. I also thought it was better keeping all layouts from this generator in layout_pools.des rather than mixing things into layout.des -- this is the approach I'm taking with my layouts, they're generally grouped by the type of generator rather than which branch they're for, since so often layouts apply to several places. I haven't added anything to layout.des, preferring to keep that just containing all the "classic" layouts (it's a pretty big file already!)

Do all these changes make any differences (i.e. performance / layout / utility wise) other than obviously the new layout?

I'll have a proper look at this patch and test locally when I'm able to; I'd definitely like to get layout_honeycombs in; and some of the utility functions you added look pretty useful too.
infiniplex (reporter)
2013-04-21 00:00

1. I have been making small images so they take less memory and load faster. They are just the Tiles minimaps scaled down and copied into a file, so I can post bigger ones if that is wanted.

2. Functionally, the dgn_remove_isolated_glyphs and dgn_widen_paths have a few more parameters than when they were LUA functions. I thought this would make them potentially useful elsewhere. The dgn_replace_closest function is entirely new. The parameters to the dgn_add_pools function are slightly changed and I restructured the inside a bit (to use C++ features not in LUA).

3. The changes are basically translating LUA to C++. dgn_remove_isolated_glyphs, dgn_widen_paths, and dgn_add_pools were previously in layout_pools.des. I would guess that C++ is slightly faster than LUA, but I did notice any change. The dgn_widen_paths function now has a greater chance of removing walls if they border more than one path cell. I had imagined it this way all along, but the effort/benefit ratio was too high in LUA.

4. layouts.des actually changes quite a lot, with layouts both added and removed (most recently layout_dis). However, grouping by approach makes a lot of sense to me.

5. I can make a new path (cumulative or replace, as preferred) to put all these layouts in the same file. layout_honeycomb uses the random_wall_material function from layouts.des. Should I a) copy the function, b) take the call out, or c) translate the function to C++?
mumra (developer)
2013-04-22 21:07

1. I wouldn't worry so much about file size; your patch is 80k but the image is only 8k! A bigger image shouldn't have a loads bigger file size anyway due to compression, and you could always use jpeg. I have a 1080p screen and those images are too small to see, so any time benefit I enjoyed from them downloading faster is wasted by having to zoom in on the images ;)

2/3. Sounds good. C++ is of course faster, how significant that gain is depends on exactly what the function is doing. Lua is generally known for being extremely performant (although in my own stuff I've discovered certain code patterns can perform really badly).

4. Historically all the generators were in layouts.des, it's only more recently that layout_loops.des and layout_vaults.des existed, but I think it's good to think about how to better organise the files since layout.des was getting rather crowded anyway. It makes sense to have layout_vaults in its own file because that's a particular type of generator and only those layouts are used in Vaults. When I was making more layouts for other places I started off doing layout_[branch].des but this quickly became impractical because often a given generator is good for a few different branches, perhaps with some small tweaks. Following the layout_loops.des example and grouping things by the type of generator seems a very nice way because someone looking at that file can hopefully understand all the layouts there once they understand one. I think layout.des would still be appropriate for very standalone and simple layouts (e.g. rwbarton's layout_dis).

5. Would be preferable to have a replacement patch, this way it'll be clearer what things changed in layout_pools. Just copy random_wall_material over, it's a really simple function, I don't see much to gain by putting it in a library (although if I end up using it in a bunch of my own layouts, which I probably should, I could always move it to a dlua/* include).
infiniplex (reporter)
2013-04-23 05:25

Here we are. The same patch, but with the file as layout_pools.
mumra (developer)
2013-04-23 11:55

Thanks! Now in trunk.

I enabled layout_honeycomb in Snake and Dis too because I feel the theme works well there. It's sometimes nice to have branch-specific variations so if you can think of anything that'd be a nice tweak - for instance, the Dis version might benefit from more squarish room shapes although as it stands the borders quite often produce pronounced and squarish corner features that work well in metal.

Also see my subsequent modification to layout_dis using the same room connection method.
sgrunt (administrator)
2013-04-24 02:06

Marking as resolved, as mumra landed this.

- Issue History
Date Modified Username Field Change
2013-04-18 06:00 infiniplex New Issue
2013-04-18 06:00 infiniplex File Added: 0001-Translated-pools-layout-code-to-C.patch
2013-04-18 06:00 infiniplex File Added: layout_honeycomb.png
2013-04-18 12:08 mumra Note Added: 0022333
2013-04-21 00:00 infiniplex Note Added: 0022362
2013-04-22 21:07 mumra Note Added: 0022387
2013-04-23 05:20 infiniplex File Added: 0001-Translated-pools-layout-code-to-C2.patch
2013-04-23 05:25 infiniplex Note Added: 0022394
2013-04-23 11:55 mumra Note Added: 0022397
2013-04-24 02:06 sgrunt Note Added: 0022411
2013-04-24 02:06 sgrunt Assigned To => mumra
2013-04-24 02:06 sgrunt Status new => resolved
2013-04-24 02:06 sgrunt Resolution open => done
2013-04-24 02:06 sgrunt Fixed in Branch => 0.13 development branch

Mantis 1.1.8[^]
Copyright © 2000 - 2009 Mantis Group
Powered by Mantis Bugtracker