Skip to content

Commit 08d7023

Browse files
matwillimatwilli
committed
CollectionModule optimization incorrectly replaced VariableModule non-1 ReportCount with 1.
Now, when combining VariableModules, ReportCount is always respected
1 parent f5d9411 commit 08d7023

3 files changed

Lines changed: 72 additions & 16 deletions

File tree

‎Waratah/HidEngine/ReportDescriptorComposition/Modules/CollectionModule.cs‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -155,12 +155,12 @@ private List<ShortItem> GenerateChildDescriptorItems(bool optimize)
155155
{
156156
// This item can't be descriptor-combined.
157157
// Add any known descriptor-combin'able items, and then restart the process again.
158-
childDescriptorItems.AddRange(this.GenerateDescriptorItemsForCombinableVariableItems(combinableItemsBuffer));
158+
childDescriptorItems.AddRange(this.GenerateDescriptorItemsForCombinableVariableItems(optimize, combinableItemsBuffer));
159159
combinableItemsBuffer.Clear();
160160

161-
if (item.GetType() == typeof(VariableModule))
161+
if (item is VariableModule castModule)
162162
{
163-
combinableItemsBuffer.Add((VariableModule)(item));
163+
combinableItemsBuffer.Add(castModule);
164164
}
165165
else
166166
{
@@ -170,7 +170,7 @@ private List<ShortItem> GenerateChildDescriptorItems(bool optimize)
170170
}
171171

172172
// Add anything remaining in the buffer.
173-
childDescriptorItems.AddRange(this.GenerateDescriptorItemsForCombinableVariableItems(combinableItemsBuffer));
173+
childDescriptorItems.AddRange(this.GenerateDescriptorItemsForCombinableVariableItems(optimize, combinableItemsBuffer));
174174
}
175175
else
176176
{
@@ -185,22 +185,20 @@ private List<ShortItem> GenerateChildDescriptorItems(bool optimize)
185185

186186
private bool IsCanBeDescriptorCombinedWithPrevious(BaseModule item, List<VariableModule> combinableItemsBuffer)
187187
{
188-
if (item.GetType() == typeof(VariableModule))
188+
if (item is VariableModule castModule)
189189
{
190-
VariableModule castItem = (VariableModule)(item);
191-
192190
// Nothing in the buffer to combine with.
193191
if (combinableItemsBuffer.Count == 0)
194192
{
195-
return castItem.IsCanBeDescriptorCombined();
193+
return castModule.IsCanBeDescriptorCombined();
196194
}
197195

198196
// Validate it can be combined will all other items in the buffer.
199197
// Note: should only really need to validate against 1 of them and the combinable property is transitive.
200198
bool isCanBeCombined = true;
201199
foreach (VariableModule combinableItem in combinableItemsBuffer)
202200
{
203-
isCanBeCombined &= combinableItem.IsCanBeDescriptorCombined(castItem);
201+
isCanBeCombined &= combinableItem.IsCanBeDescriptorCombined(castModule);
204202
}
205203

206204
return isCanBeCombined;
@@ -209,11 +207,15 @@ private bool IsCanBeDescriptorCombinedWithPrevious(BaseModule item, List<Variabl
209207
return false;
210208
}
211209

212-
private List<ShortItem> GenerateDescriptorItemsForCombinableVariableItems(List<VariableModule> combinableItemsBuffer)
210+
private List<ShortItem> GenerateDescriptorItemsForCombinableVariableItems(bool optimize, List<VariableModule> combinableItemsBuffer)
213211
{
214212
List<ShortItem> descriptorItems = new List<ShortItem>();
215213

216-
if (combinableItemsBuffer.Count > 0)
214+
if (combinableItemsBuffer.Count == 1)
215+
{
216+
descriptorItems.AddRange(combinableItemsBuffer[0].GenerateDescriptorItems(optimize));
217+
}
218+
else if (combinableItemsBuffer.Count >= 1)
217219
{
218220
List<HidUsageId> combinableUsages = new List<HidUsageId>();
219221

‎Waratah/HidEngine/ReportDescriptorComposition/Modules/VariableModule.cs‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
namespace HidEngine.ReportDescriptorComposition.Modules
55
{
66
using System.Collections.Generic;
7+
using System.Linq;
78
using HidEngine.Properties;
89
using HidEngine.ReportDescriptorItems;
910
using HidSpecification;
@@ -77,9 +78,9 @@ public override List<ShortItem> GenerateDescriptorItems(bool optimize)
7778
/// </code>
7879
/// </summary>
7980
/// <param name="combineUsages">Usages to combine together into a variable item.</param>
80-
/// <param name="reportCount">Counts to combine.</param>
81+
/// <param name="combinedReportCount">Combined ReportCount for this item.</param>
8182
/// <returns>List of HID Report Items describing this <see cref="VariableModule"/>.</returns>
82-
public List<ShortItem> GenerateDescriptorItems(List<HidUsageId> combineUsages, int reportCount)
83+
public List<ShortItem> GenerateDescriptorItems(List<HidUsageId> combineUsages, int combinedReportCount)
8384
{
8485
List<ShortItem> descriptorItems = new List<ShortItem>();
8586

@@ -93,9 +94,9 @@ public List<ShortItem> GenerateDescriptorItems(List<HidUsageId> combineUsages, i
9394

9495
int cachedCount = this.Count;
9596

96-
// Temporarily set the Count to the # items that are being combined.
97+
// Temporarily set the base Count to the # items that are being combined.
9798
// This allows the existing generation logic to work.
98-
this.Count = reportCount;
99+
this.Count = combinedReportCount;
99100

100101
descriptorItems.AddRange(this.GenerateReportDataVariableItems(HidConstants.MainDataItemGroupingKind.Variable));
101102

@@ -123,7 +124,7 @@ public bool IsCanBeDescriptorCombined()
123124
public bool IsCanBeDescriptorCombined(VariableModule other)
124125
{
125126
// Can only be descriptor-combined if this corresponds to a single field.
126-
bool isCanBeCombined = other.IsCanBeDescriptorCombined();
127+
bool isCanBeCombined = (other.IsCanBeDescriptorCombined() == this.IsCanBeDescriptorCombined());
127128

128129
// Validate all other fields are the same, except for UsageId; where it being different is the whole point...
129130
bool isSameUsagePage = (other.Usage.Page.Id == this.Usage.Page.Id);

‎Waratah/HidEngineTest/ReportDescriptorComposition/CollectionModuleTests.cs‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ namespace HidEngineTest.ReportDescriptorComposition
1010
using HidSpecification;
1111
using Microsoft.VisualStudio.TestTools.UnitTesting;
1212
using System.Collections.Generic;
13+
using System.Linq;
1314

1415
[TestClass]
1516
public class CollectionModuleTests
@@ -39,5 +40,57 @@ public void SimpleCollection()
3940
Assert.AreEqual(HidConstants.MainItemCollectionKind.Logical, logicalCollection.Kind);
4041
Assert.AreEqual((array.TotalSizeInBits + variable.TotalSizeInBits), logicalCollection.TotalSizeInBits);
4142
}
43+
44+
/// <summary>
45+
/// Modules will be combined when all fields are identical (and ReportCount == 1). This reduces the number of descriptor items.
46+
/// The combined module will have all the same fields, except, multiple UsageIds and enlarged ReportCount
47+
/// </summary>
48+
[TestMethod]
49+
public void OptimizedCollectionsCombineFields()
50+
{
51+
// Two VariableModules with ReportCount==1, preceed VariableModule with ReportCount==3
52+
// First two VariableModules will be combined.
53+
{
54+
ReportModule report = new ReportModule(DefaultReportKind, null);
55+
56+
// Using LogicalCollection, as is a simple specialization of BaseCollection.
57+
CollectionModule logicalCollection = new CollectionModule(HidConstants.MainItemCollectionKind.Logical, report);
58+
59+
DescriptorRange logicalRange = new DescriptorRange(0, 10);
60+
VariableModule variable1 = new VariableModule(HidUsageTableDefinitions.GetInstance().TryFindUsageId("Ordinal", "Instance 1"), 1, logicalRange, null, null, null, null, null, string.Empty, report);
61+
VariableModule variable2 = new VariableModule(HidUsageTableDefinitions.GetInstance().TryFindUsageId("Ordinal", "Instance 2"), 1, logicalRange, null, null, null, null, null, string.Empty, report);
62+
VariableModule variable3 = new VariableModule(HidUsageTableDefinitions.GetInstance().TryFindUsageId("Ordinal", "Instance 3"), 3, logicalRange, null, null, null, null, null, string.Empty, report);
63+
64+
logicalCollection.Initialize(DefaultCollectionUsage, new List<BaseModule> { variable1, variable2, variable3 });
65+
66+
List<ShortItem> generatedItems = logicalCollection.GenerateDescriptorItems(true);
67+
68+
uint[] foundReportItemCountValue = generatedItems.Where(x => x is ReportCountItem).Select(x => ((ReportCountItem)x).Count).ToArray();
69+
70+
CollectionAssert.AreEqual(foundReportItemCountValue, new uint[] { 2, 3 });
71+
}
72+
73+
// Two VariableModules with ReportCount==1, after VariableModule with ReportCount==3
74+
// Last two VariableModules will be combined.
75+
{
76+
ReportModule report = new ReportModule(DefaultReportKind, null);
77+
78+
// Using LogicalCollection, as is a simple specialization of BaseCollection.
79+
CollectionModule logicalCollection = new CollectionModule(HidConstants.MainItemCollectionKind.Logical, report);
80+
81+
DescriptorRange logicalRange = new DescriptorRange(0, 10);
82+
VariableModule variable1 = new VariableModule(HidUsageTableDefinitions.GetInstance().TryFindUsageId("Ordinal", "Instance 1"), 1, logicalRange, null, null, null, null, null, string.Empty, report);
83+
VariableModule variable2 = new VariableModule(HidUsageTableDefinitions.GetInstance().TryFindUsageId("Ordinal", "Instance 2"), 1, logicalRange, null, null, null, null, null, string.Empty, report);
84+
VariableModule variable3 = new VariableModule(HidUsageTableDefinitions.GetInstance().TryFindUsageId("Ordinal", "Instance 3"), 3, logicalRange, null, null, null, null, null, string.Empty, report);
85+
86+
logicalCollection.Initialize(DefaultCollectionUsage, new List<BaseModule> {variable3, variable1, variable2});
87+
88+
List<ShortItem> generatedItems = logicalCollection.GenerateDescriptorItems(true);
89+
90+
uint[] foundReportItemCountValue = generatedItems.Where(x => x is ReportCountItem).Select(x => ((ReportCountItem)x).Count).ToArray();
91+
92+
CollectionAssert.AreEqual(foundReportItemCountValue, new uint[] { 3, 2 });
93+
}
94+
}
4295
}
4396
}

0 commit comments

Comments
 (0)