Improve Transports To Load Default #647
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,6 @@ | |
| import games.strategy.triplea.delegate.TransportTracker; | ||
| import games.strategy.util.IntegerMap; | ||
| import games.strategy.util.Match; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.Collection; | ||
|
|
@@ -17,16 +16,13 @@ | |
| import java.util.Iterator; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Optional; | ||
|
|
||
|
|
||
| public class TransportUtils { | ||
|
There was a problem hiding this comment. The diff of this class is pretty much unreadable do to several methods being refactored and moved around. Suggest either looking over the final class or review commit by commit. |
||
|
|
||
| /** | ||
| * Returns a map of unit -> transport. | ||
| */ | ||
| public static Map<Unit, Unit> mapTransports(final Route route, final Collection<Unit> units, | ||
| final Collection<Unit> transportsToLoad) { | ||
|
|
@@ -40,125 +36,55 @@ public static Map<Unit, Unit> mapTransports(final Route route, final Collection< | |
| } | ||
|
|
||
| /** | ||
| * Returns a map of unit -> transport. Tries to load units evenly across all transports. | ||
| */ | ||
| public static Map<Unit, Unit> mapTransportsToLoad(final Collection<Unit> units, | ||
| final Collection<Unit> transports) { | ||
|
|
||
| final List<Unit> canBeTransported = sortByTransportCostDescending(units); | ||
| final List<Unit> canTransport = sortByTransportCapacityAscending(transports); | ||
|
|
||
| // Add units to transports evenly | ||
| final Map<Unit, Unit> mapping = new HashMap<Unit, Unit>(); | ||
| final IntegerMap<Unit> addedLoad = new IntegerMap<Unit>(); | ||
| for (final Unit unit : canBeTransported) { | ||
|
There was a problem hiding this comment. Complexity looks to be getting up there in this method. Can we push some of the lower level logic into helper methods? Java8 streams + filter operations might be another option, the inner most for loop looks like it can be simplified to a stream, filter, find_first operation There was a problem hiding this comment. Considered using streams but given the number of variables being tracked and needing to move the loaded transport to the end of the list, I couldn't think of a good way to do that. I'll take a look to see if a helper method makes sense. There was a problem hiding this comment. Ended up creating a helper method. I think it helps out a little though it isn't the best helper method as it has 4 parameters and modifies 2 of them. There was a problem hiding this comment. The helper method certainly helped |
||
| final Optional<Unit> transport = loadUnitIntoFirstAvailableTransport(unit, canTransport, mapping, addedLoad); | ||
|
|
||
| // Move loaded transport to end of list | ||
|
There was a problem hiding this comment. Instead of moving the loaded transport to the end of the list, could we check if it is full, and if so, remove it? There was a problem hiding this comment. Yeah, I originally had that in and decided to remove it since performance wasn't really an issue. Figured its better to keep it as simple as possible. This is kind of the age old problem of having to edit a list you are looping through. Essentially after a unit is added, the transport needs to be moved to the end of the list. Any thoughts on a better way to move the loaded transport to the end of the list other than removing then adding it? There was a problem hiding this comment. IMO avoid the need to move it to the end. Alternatively have an index point where you start your iterations. This would mean converting your inner foreach loop into an old school style |
||
| if (transport.isPresent()) { | ||
|
There was a problem hiding this comment. I suspect you can simplify, everything should be in memory, order nanosecond runtimes. This performance improvement strikes me each time. |
||
| canTransport.remove(transport.get()); | ||
| canTransport.add(transport.get()); | ||
| } | ||
| } | ||
| return mapping; | ||
| } | ||
|
|
||
| /** | ||
| * Returns a map of unit -> transport. Tries load max units into each transport before moving to next. | ||
| */ | ||
| public static Map<Unit, Unit> mapTransportsToLoadUsingMinTransports(final Collection<Unit> units, | ||
| final Collection<Unit> transports) { | ||
|
|
||
| final List<Unit> canBeTransported = sortByTransportCostDescending(units); | ||
| final List<Unit> canTransport = sortByTransportCapacityAscending(transports); | ||
|
|
||
| // Add max units to each transport | ||
| final Map<Unit, Unit> mapping = new HashMap<Unit, Unit>(); | ||
| for (final Unit transport : canTransport) { | ||
| int capacity = TransportTracker.getAvailableCapacity(transport); | ||
| for (final Iterator<Unit> it = canBeTransported.iterator(); it.hasNext();) { | ||
| final Unit unit = it.next(); | ||
| final int cost = UnitAttachment.get((unit).getType()).getTransportCost(); | ||
| if (capacity >= cost) { | ||
| capacity -= cost; | ||
| mapping.put(unit, transport); | ||
| it.remove(); | ||
| } | ||
| } | ||
| } | ||
| return mapping; | ||
| } | ||
|
|
||
| public static List<Unit> findUnitsToLoadOnAirTransports(final Collection<Unit> units, | ||
| final Collection<Unit> transports) { | ||
| final Collection<Unit> airTransports = Match.getMatches(transports, Matches.UnitIsAirTransport); | ||
|
|
@@ -194,4 +120,84 @@ public int compare(final Unit o1, final Unit o2) { | |
| return totalLoad; | ||
| } | ||
|
|
||
| public static int getTransportCost(final Collection<Unit> units) { | ||
| if (units == null) { | ||
| return 0; | ||
| } | ||
| int cost = 0; | ||
| final Iterator<Unit> iter = units.iterator(); | ||
| while (iter.hasNext()) { | ||
| final Unit item = iter.next(); | ||
| cost += UnitAttachment.get(item.getType()).getTransportCost(); | ||
| } | ||
| return cost; | ||
| } | ||
|
|
||
| /** | ||
| * Returns a map of unit -> transport. Unit must already be loaded in the transport. | ||
| */ | ||
| private static Map<Unit, Unit> mapTransportsAlreadyLoaded(final Collection<Unit> units, | ||
| final Collection<Unit> transports) { | ||
| final Collection<Unit> canBeTransported = Match.getMatches(units, Matches.UnitCanBeTransported); | ||
| final Collection<Unit> canTransport = Match.getMatches(transports, Matches.UnitCanTransport); | ||
| final Map<Unit, Unit> mapping = new HashMap<Unit, Unit>(); | ||
| final Iterator<Unit> land = canBeTransported.iterator(); | ||
|
There was a problem hiding this comment. The complexity is not trivial in this method. Can you avoid using Iterator? Looks like a foreach or a Java8 stream+filter would both work to simplify things. There was a problem hiding this comment. Actually didn't touch this method. Just the mapTransportToLoad* ones. Leaving it alone for now unless I end up needing to update it for other transport enhancements. |
||
| while (land.hasNext()) { | ||
| final Unit currentTransported = land.next(); | ||
| final Unit transport = TransportTracker.transportedBy(currentTransported); | ||
|
|
||
| // already being transported, make sure it is in transports | ||
| if (transport == null) { | ||
| continue; | ||
| } | ||
| if (!canTransport.contains(transport)) { | ||
| continue; | ||
| } | ||
| mapping.put(currentTransported, transport); | ||
| } | ||
| return mapping; | ||
| } | ||
|
|
||
| private static List<Unit> sortByTransportCapacityAscending(final Collection<Unit> transports) { | ||
| final Comparator<Unit> transportCapacityComparator = new Comparator<Unit>() { | ||
| @Override | ||
| public int compare(final Unit o1, final Unit o2) { | ||
| final int capacityLeft1 = TransportTracker.getAvailableCapacity(o1); | ||
| final int capacityLeft2 = TransportTracker.getAvailableCapacity(o2); | ||
| return capacityLeft1 - capacityLeft2; | ||
| } | ||
| }; | ||
| final List<Unit> canTransport = Match.getMatches(transports, Matches.UnitCanTransport); | ||
| Collections.sort(canTransport, transportCapacityComparator); | ||
| return canTransport; | ||
| } | ||
|
|
||
| private static List<Unit> sortByTransportCostDescending(final Collection<Unit> units) { | ||
| final Comparator<Unit> transportCostComparator = new Comparator<Unit>() { | ||
| @Override | ||
| public int compare(final Unit o1, final Unit o2) { | ||
| final int cost1 = UnitAttachment.get((o1).getUnitType()).getTransportCost(); | ||
| final int cost2 = UnitAttachment.get((o2).getUnitType()).getTransportCost(); | ||
| return cost2 - cost1; | ||
| } | ||
| }; | ||
| final List<Unit> canBeTransported = Match.getMatches(units, Matches.UnitCanBeTransported); | ||
|
There was a problem hiding this comment. @RoiEXLab - example of Matching code |
||
| Collections.sort(canBeTransported, transportCostComparator); | ||
| return canBeTransported; | ||
| } | ||
|
|
||
| private static Optional<Unit> loadUnitIntoFirstAvailableTransport(final Unit unit, final List<Unit> canTransport, | ||
|
|
||
| final Map<Unit, Unit> mapping, final IntegerMap<Unit> addedLoad) { | ||
| final int cost = UnitAttachment.get((unit).getType()).getTransportCost(); | ||
| for (final Unit transport : canTransport) { | ||
| final int capacity = TransportTracker.getAvailableCapacity(transport) - addedLoad.getInt(transport); | ||
| if (capacity >= cost) { | ||
| addedLoad.add(transport, cost); | ||
| mapping.put(unit, transport); | ||
| return Optional.of(transport); | ||
| } | ||
| } | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this method to TransportUtils.