From 1544f6d4906b57311c14dd7767f106870ba207b9 Mon Sep 17 00:00:00 2001 From: Greenfire27 Date: Mon, 17 Aug 2026 21:14:20 -0400 Subject: [PATCH 1/3] A new project that is named after your game Every project this editor has ever made had a module called BlankGame in it. NewProjectDialog handed CopyModule the same id for source and target, so the engine's rename path never ran, and the game module kept the template's name, the template's Author of nothing, and the template's description -- "A blank game, ready for you to craft into something amazing!" -- no matter what the person creating it typed. The dialog asked for three things and only the title reached anything. It asks for six now: Title, Directory, Game Core, Module Name, Author and Description, each with a tooltip on the caption as well as on the input. Module Name defaults to the title stripped to identifier characters plus "Game", and it follows the title until the moment someone types their own, after which it is theirs. Description now reaches the game module as well as AppCore, and Author reaches it at all. RENAMING A MODULE IS NOT RENAMING ITS ModuleId. The engine calls ::, so the id in module.taml and the namespace in the module's script are the same name written twice, and asset ids are that name a third time -- ":", in script and in every taml that references an asset. Rewrite only module.taml and the module still loads, still reports its new name everywhere the UI looks, and silently does nothing: create never fires, and its assets resolve to a module that no longer exists. Nothing about the definition it wrote looked wrong, which is why this went unnoticed. So the rename is a pass over the module's own source, and it lives in one place, ModuleStamper, called from all three paths that rename a module. The other two were worse than the New Project one, because a person could reach them repeatedly: New Module from a template (ProjectGamePanel::onModuleCreated) and Edit Module changing a name (::onModuleEdited) both rewrote ModuleID and stopped there. The substitution is whole-word -- a match counts only where neither neighbour can be part of an identifier -- because in the Edit Module case the old id is whatever a person called their module, and can be a substring of an ordinary word in a comment. The engine has half of this already and it is not enough. copyModule runs a TamlModuleIdUpdateVisitor when the ids differ, but that visitor is root-only, so an asset id on a nested element is missed; it cannot touch .cs at all, which is where the namespace lives; and it renames module.taml to .module.taml, a name that nothing else in the editor opens a module definition by, with no fileRename bound to script to put it back. Hence the copy going out under the template's own id and the rename happening afterwards, on the copy. TEMPLATE IS NOW A FLAG AND Type SAYS WHAT A MODULE IS. Type == "Template" was only ever read as an enumeration filter, in the two places that either wanted templates or did not; "leave this module alone" is enforced by the engine's own Synchronized field and always was. So BlankGame is Type="Game Core" and ArtPack is Type="Art Pack", both carrying Template="1" and a DisplayName, the two filters test the flag, and the Game Core dropdown can ask findModuleTypes for exactly the templates a project can be built out of. The markers come off the copy. Two fixes to what was already there, both found by the tests. The New Project dialog appended a trailing backslash to its project path and then built every other path on top of it, which the engine's own file calls expand away and script-side isDirectory does not -- it stats the string it is given, so ModuleStamper was handed a folder that did not exist. And the buttons were positioned from the dialog's own height rather than from EditorDialog:: contentHeight, putting the Create button eight pixels past the fold and the whole form behind a scroll bar; the comment on contentHeight names this exact trap and lists two other dialogs that shipped with it. Both suites end by loading the renamed module and checking its gui reached the Canvas, because that is the only thing that proves create actually fired. The layout checks assert every control ends inside the content pane, and they fail on the code that shipped a scroll bar. Co-Authored-By: Claude Opus 5 (1M context) --- editor/EditorCore/EditorCore.cs | 11 + editor/EditorCore/EditorForm.cs | 16 ++ editor/EditorCore/ModuleStamper.cs | 226 ++++++++++++++++ .../scripts/EditorProjectSelector.cs | 7 +- editor/EditorCore/scripts/NewProjectDialog.cs | 227 ++++++++++++++-- .../ProjectManager/scripts/NewModuleDialog.cs | 25 +- .../scripts/ProjectGamePanel.cs | 21 ++ .../scripts/ProjectLibraryPanel.cs | 4 +- library/ArtPack/module.taml | 4 +- library/BlankGame/module.taml | 4 +- tests/smoke/moduleRename.cs | 198 ++++++++++++++ tests/smoke/newProject.cs | 245 ++++++++++++++++++ 12 files changed, 957 insertions(+), 31 deletions(-) create mode 100644 editor/EditorCore/ModuleStamper.cs create mode 100644 tests/smoke/moduleRename.cs create mode 100644 tests/smoke/newProject.cs diff --git a/editor/EditorCore/EditorCore.cs b/editor/EditorCore/EditorCore.cs index 1416aacb4..80433924c 100644 --- a/editor/EditorCore/EditorCore.cs +++ b/editor/EditorCore/EditorCore.cs @@ -77,11 +77,17 @@ exec("./EditorAssetPickerItem.cs"); exec("./EditorPreferences.cs"); + // Out here rather than with NewProjectDialog above, because the Project + // Manager renames modules too and it is loaded once a project is open. + exec("./ModuleStamper.cs"); + new ScriptObject(ThemeManager); // Before any editor builds a control that wants to remember how it was left. new ScriptObject(EditorPreferences); + new ScriptObject(ModuleStamper); + %this.initGui(); %this.editorKeyMap.push(); } @@ -95,6 +101,11 @@ EditorPreferences.delete(); } + if(isObject(ModuleStamper)) + { + ModuleStamper.delete(); + } + // Empty by now - every editor deletes its own menus before this runs, and // each module is unloaded before the one it depends on. if(isObject(%this.menuPark)) diff --git a/editor/EditorCore/EditorForm.cs b/editor/EditorCore/EditorForm.cs index 921ce6fe6..8db278222 100644 --- a/editor/EditorCore/EditorForm.cs +++ b/editor/EditorCore/EditorForm.cs @@ -42,6 +42,22 @@ return %label; } +// Explains a row on hover. The tip goes on the caption as well as on the input, +// because the caption is the bigger target and it is what someone is looking at +// when the question comes up -- and the caption is the input's parent, so +// without both, half the row says nothing. +function EditorForm::setItemTip(%this, %label, %control, %tip) +{ + %label.Tooltip = %tip; + ThemeManager.setProfile(%label, "tipProfile", "TooltipProfile"); + + if(isObject(%control)) + { + %control.Tooltip = %tip; + ThemeManager.setProfile(%control, "tipProfile", "TooltipProfile"); + } +} + function EditorForm::createTextEditItem(%this, %label) { %textEdit = new GuiTextEditCtrl() diff --git a/editor/EditorCore/ModuleStamper.cs b/editor/EditorCore/ModuleStamper.cs new file mode 100644 index 000000000..c0ea9e4d2 --- /dev/null +++ b/editor/EditorCore/ModuleStamper.cs @@ -0,0 +1,226 @@ +//----------------------------------------------------------------------------- +// Renaming a module is not renaming its ModuleId. +// +// The engine calls ::, so the id in module.taml and +// the namespace in the module's script are the same name written twice. Asset +// ids are that name a third time: ":", in script and in +// every taml file that references an asset. Change only the one in module.taml +// -- which is what all three of the editor's rename paths used to do -- and the +// module still loads, still reports the new name, and silently does nothing: +// create never fires, and its assets resolve to a module that no longer exists. +// +// So a rename is a pass over the module's own source. Only .cs and .taml files +// are read; art and audio are left alone. +// +// The engine has half of this already: ModuleManager::copyModule runs a +// TamlModuleIdUpdateVisitor when the source and target ids differ. It is not +// enough on its own -- the visitor is root-only, so an asset id on a nested +// element is missed, it cannot touch .cs at all, and it renames module.taml to +// .module.taml, a name every editor script that opens a module +// definition does not expect. The copy is therefore made under the template's +// own id and the rename happens here, on the copy. +//----------------------------------------------------------------------------- + +// The extensions worth reading. Everything else in a module is content. +function ModuleStamper::onAdd(%this) +{ + %this.textExtensions = ".cs" TAB ".taml"; +} + +// Template modules carry two dynamic fields the engine knows nothing about: +// Template marks a module as something to stamp out rather than install, and +// DisplayName is what to call it in a picker. Neither is a ModuleDefinition +// field, so both ride along as taml attributes the way AppCore's Project and +// ProjectDescription do. The names live here so the dialogs that read them and +// the code that strips them off a copy agree on the spelling. +function ModuleStamper::displayName(%this, %module) +{ + if(%module.DisplayName !$= "") + { + return %module.DisplayName; + } + + return %module.ModuleID; +} + +// A stamped copy is a module in its own right, not a template, so the markers +// that made it stampable do not belong on it. +function ModuleStamper::clearTemplateMarkers(%this, %definition) +{ + %definition.Template = ""; + %definition.DisplayName = ""; +} + +// %modulePath is the module's folder; %oldId and %newId are module ids. Returns +// true if the walk completed, whether or not any file needed changing. +function ModuleStamper::renameInPlace(%this, %modulePath, %oldId, %newId) +{ + if(%oldId $= "" || %newId $= "" || %oldId $= %newId) + { + return true; + } + + if(!isDirectory(%modulePath)) + { + error("ModuleStamper: no module at " @ %modulePath); + return false; + } + + return %this.rewriteTree(%modulePath, %oldId, %newId); +} + +// One level at a time rather than getDirectoryList's depth argument: that +// binding passes noBasePath, so the base folder is never in the list and the +// returned names are relative to it. Recursing by hand keeps the full path in +// hand at every level. +function ModuleStamper::rewriteTree(%this, %dir, %old, %new) +{ + %files = getFileList(%dir); + for(%i = 0; %i < getFieldCount(%files); %i++) + { + %file = getField(%files, %i); + if(%this.isTextFile(%file)) + { + %this.rewriteFile(pathConcat(%dir, %file), %old, %new); + } + } + + %dirs = getDirectoryList(%dir); + for(%i = 0; %i < getFieldCount(%dirs); %i++) + { + %sub = getField(%dirs, %i); + if(%sub $= "" || %sub $= "." || %sub $= "..") + { + continue; + } + + %this.rewriteTree(pathConcat(%dir, %sub), %old, %new); + } + + return true; +} + +function ModuleStamper::isTextFile(%this, %file) +{ + %ext = fileExt(%file); + for(%i = 0; %i < getFieldCount(%this.textExtensions); %i++) + { + if(%ext $= getField(%this.textExtensions, %i)) + { + return true; + } + } + + return false; +} + +// Read the whole file before writing any of it. FileObject reads through the +// ResourceManager, which caches a file's size the first time it is asked for +// one, so a file read back after being written in the same session can be read +// at its old length. +function ModuleStamper::rewriteFile(%this, %path, %old, %new) +{ + %file = new FileObject(); + if(!%file.openForRead(%path)) + { + %file.delete(); + error("ModuleStamper: could not read " @ %path); + return false; + } + + %count = 0; + %changed = false; + while(!%file.isEOF()) + { + %line = %file.readLine(); + %rewritten = %this.replaceToken(%line, %old, %new); + if(%rewritten !$= %line) + { + %changed = true; + } + + %out[%count] = %rewritten; + %count++; + } + %file.close(); + + if(!%changed) + { + %file.delete(); + return true; + } + + if(!%file.openForWrite(%path)) + { + %file.delete(); + error("ModuleStamper: could not write " @ %path); + return false; + } + + for(%i = 0; %i < %count; %i++) + { + %file.writeLine(%out[%i]); + } + %file.close(); + %file.delete(); + + return true; +} + +// A whole-word replace. strreplace would do for "BlankGame", but this also runs +// over modules a person named themselves, where the old id can be a substring +// of an ordinary word in a comment or a string. A match counts only where the +// characters on either side cannot be part of an identifier -- which the three +// forms that matter all satisfy: ModuleId="Name", Name::create, "Name:asset". +function ModuleStamper::replaceToken(%this, %line, %old, %new) +{ + %length = strlen(%old); + if(%length == 0) + { + return %line; + } + + %result = ""; + %from = 0; + + while(true) + { + %at = strpos(%line, %old, %from); + if(%at == -1) + { + return %result @ getSubStr(%line, %from, strlen(%line) - %from); + } + + %before = (%at == 0) ? "" : getSubStr(%line, %at - 1, 1); + %after = getSubStr(%line, %at + %length, 1); + + %result = %result @ getSubStr(%line, %from, %at - %from); + if(%this.isIdentifierChar(%before) || %this.isIdentifierChar(%after)) + { + %result = %result @ %old; + } + else + { + %result = %result @ %new; + } + + %from = %at + %length; + } +} + +function ModuleStamper::isIdentifierChar(%this, %char) +{ + if(%char $= "") + { + return false; + } + + if(%char $= "_") + { + return true; + } + + // $= is case insensitive, so the lower case half of the alphabet answers for + // both. + return strpos("abcdefghijklmnopqrstuvwxyz0123456789", strlwr(%char)) != -1; +} diff --git a/editor/EditorCore/scripts/EditorProjectSelector.cs b/editor/EditorCore/scripts/EditorProjectSelector.cs index 53476a492..b66ec64a6 100644 --- a/editor/EditorCore/scripts/EditorProjectSelector.cs +++ b/editor/EditorCore/scripts/EditorProjectSelector.cs @@ -188,14 +188,19 @@ function EditorProjectSelector::onNewProject(%this) { + // Six 50 pixel form rows, the feedback line, and the button row, plus the 34 + // pixels the window keeps for its title bar and border. A fixed form gains + // nothing from being dragged bigger and loses the bottom of itself behind a + // scroll bar when dragged smaller, so it does not resize. %width = 700; - %height = 340; + %height = 470; %dialog = new GuiControl() { class = "NewProjectDialog"; superclass = "EditorDialog"; dialogSize = (%width + 8) SPC (%height + 8); dialogCanClose = true; + dialogResizable = false; dialogText = "New Project"; }; %dialog.init(%width, %height); diff --git a/editor/EditorCore/scripts/NewProjectDialog.cs b/editor/EditorCore/scripts/NewProjectDialog.cs index 3214fa157..0e4b3e137 100644 --- a/editor/EditorCore/scripts/NewProjectDialog.cs +++ b/editor/EditorCore/scripts/NewProjectDialog.cs @@ -17,24 +17,45 @@ class = "EditorForm"; %item = %form.addFormItem("Title", %width SPC 30); %this.titleBox = %form.createTextEditItem(%item); - %this.titleBox.Command = %this.getId() @ ".Validate();"; + %form.setItemTip(%item, %this.titleBox, "What your project is called. This is the name on its card in the project picker, so write it for people to read - spaces and punctuation are fine."); %item = %form.addFormItem("Directory", %width SPC 30); %this.dirBox = %form.createTextEditItem(%item); - %this.dirBox.Command = %this.getId() @ ".Validate();"; + %form.setItemTip(%item, %this.dirBox, "The folder your project lives in, made inside the Torque2D folder. It has to be empty or not exist yet, so nothing you already have is written over."); - %item = %form.addFormItem("Description", %width SPC 130); + %item = %form.addFormItem("Game Core", %width SPC 30); + %this.coreDropDown = %form.createDropDownItem(%item); + %this.populateGameCores(); + %form.setItemTip(%item, %this.coreDropDown, "Which template your game is copied from. Blank Game gives you a window, a background and some music, ready to be replaced with your own."); + + %item = %form.addFormItem("Module Name", %width SPC 30); + %this.moduleNameBox = %form.createTextEditItem(%item); + %form.setItemTip(%item, %this.moduleNameBox, "What to call your game module. It becomes the module's folder, its ModuleId, and the namespace your game's create and destroy functions hang off - so letters, numbers and underscores only. It follows the title until you edit it."); + + %item = %form.addFormItem("Author", %width SPC 30); + %this.authorBox = %form.createTextEditItem(%item); + %form.setItemTip(%item, %this.authorBox, "Who to credit for the game module. Optional: left empty, the Project Manager credits Torque2D. You can change it later under Edit Module."); + + %item = %form.addFormItem("Description", %width SPC 30); %this.descBox = %form.createTextEditItem(%item); - %this.descBox.Command = %this.getId() @ ".Validate();"; + %form.setItemTip(%item, %this.descBox, "A sentence saying what the project is. It shows under the title on the project card and on the game module in the Project Manager."); + %this.form = %form; %content.add(%form); + // Positioned from the room the content actually gets rather than from the + // dialog's own height: the window spends 34 pixels of it on the title bar and + // its border (EditorDialog::contentHeight), and a button placed past that puts + // the whole form behind a scroll bar. + %formHeight = 6 * 50; + %buttonTop = %this.contentHeight() - 46; + %this.feedback = new GuiControl() { HorizSizing = "right"; VertSizing = "bottom"; - Position = "12 170"; - Extent = (%width - 24) SPC 80; + Position = "12" SPC (%formHeight + 10); + Extent = (%width - 24) SPC (%buttonTop - %formHeight - 28); text = ""; textWrap = true; textExtend = true; @@ -45,7 +66,7 @@ class = "EditorForm"; { HorizSizing = "right"; VertSizing = "bottom"; - Position = "478 270"; + Position = "478" SPC (%buttonTop + 2); Extent = "100 30"; Text = "Cancel"; Command = %this.getID() @ ".onClose();"; @@ -56,7 +77,7 @@ class = "EditorForm"; { HorizSizing = "right"; VertSizing = "bottom"; - Position = "588 268"; + Position = "588" SPC %buttonTop; Extent = "100 34"; Text = "Create"; Command = %this.getID() @ ".onCreate();"; @@ -70,12 +91,111 @@ class = "EditorForm"; %this.validate(); } +// The game cores are the library templates a project can be built out of. A +// private ModuleManager rather than ModuleDatabase: the project selector has +// nothing scanned at this point and this must not leave anything behind that +// would show up as a module of the project about to be created. +function NewProjectDialog::populateGameCores(%this) +{ + %manager = new ModuleManager(); + %manager.EchoInfo = false; + %manager.ScanModules(pathConcat(getMainDotCsDir(), "library")); + + %cores = %manager.findModuleTypes("Game Core", false); + for(%i = 0; %i < getWordCount(%cores); %i++) + { + %core = getWord(%cores, %i); + if(!%core.Template) + { + continue; + } + + // The dropdown shows a display name and sortByText reorders it, so the id + // is remembered against the name. The module definitions do not outlive + // the manager. + %name = ModuleStamper.displayName(%core); + %this.coreDropDown.addItem(%name); + %this.coreID[%name] = %core.ModuleID; + } + + %manager.delete(); + + %this.coreDropDown.sortByText(); + if(%this.coreDropDown.getItemCount() > 0) + { + %this.coreDropDown.setSelected(0); + } +} + +function NewProjectDialog::selectedGameCore(%this) +{ + return %this.coreID[%this.coreDropDown.getText()]; +} + +// Titles are written for people and module ids are written for the script +// compiler, so the suggestion is the title with everything a namespace cannot +// carry taken out of it. +function NewProjectDialog::suggestModuleName(%this) +{ + %title = %this.titleBox.getText(); + %name = ""; + + for(%i = 0; %i < strlen(%title); %i++) + { + %char = getSubStr(%title, %i, 1); + if(ModuleStamper.isIdentifierChar(%char)) + { + %name = %name @ %char; + } + } + + if(%name $= "") + { + return ""; + } + + return %name @ "Game"; +} + +// The module name follows the title until the moment someone types their own, +// and from then on it is theirs. setText does not run the box's command, so the +// guard is belt and braces against that changing. +function NewProjectDialog::onKeyPressed(%this, %textBox) +{ + if(%textBox == %this.moduleNameBox) + { + if(!%this.settingModuleName) + { + %this.moduleNameEdited = true; + } + } + else if(%textBox == %this.titleBox && !%this.moduleNameEdited) + { + %this.settingModuleName = true; + %this.moduleNameBox.setText(%this.suggestModuleName()); + %this.settingModuleName = false; + } + + %this.validate(); +} + +function NewProjectDialog::onReturnPressed(%this, %textBox) +{ + %this.onCreate(); +} + +function NewProjectDialog::onDropDownClosed(%this, %dropDown) +{ + %this.validate(); +} + function NewProjectDialog::Validate(%this) { %this.createButton.active = false; %title = %this.titleBox.getText(); %directory = %this.dirBox.getText(); + %moduleName = %this.moduleNameBox.getText(); %description = %this.descBox.getText(); if(%title $= "") @@ -104,6 +224,17 @@ class = "EditorForm"; } } + if(%this.selectedGameCore() $= "") + { + %this.feedback.setText("Please choose the game core to build your project from. If this list is empty, the library folder has no game core template in it."); + return false; + } + + if(!%this.validateModuleName(%moduleName)) + { + return false; + } + if(%description $= "") { %this.feedback.setText("Please add a short, meaningful description for your project."); @@ -115,26 +246,78 @@ class = "EditorForm"; return true; } +// The module name becomes a folder, a ModuleId, and the namespace the engine +// calls create and destroy on, so it has to be a legal script identifier and it +// cannot collide with the modules copied in beside it. +function NewProjectDialog::validateModuleName(%this, %moduleName) +{ + if(%moduleName $= "") + { + %this.feedback.setText("Please enter a name for your game module."); + return false; + } + + for(%i = 0; %i < strlen(%moduleName); %i++) + { + if(!ModuleStamper.isIdentifierChar(getSubStr(%moduleName, %i, 1))) + { + %this.feedback.setText("The module name becomes a script namespace, so it can only contain letters, numbers and underscores."); + return false; + } + } + + if(strpos("0123456789", getSubStr(%moduleName, 0, 1)) != -1) + { + %this.feedback.setText("The module name cannot start with a number."); + return false; + } + + if(%moduleName $= "AppCore" || %moduleName $= "Audio" || %moduleName $= "themes") + { + %this.feedback.setText("AppCore, Audio and themes are already used by every project. Please pick another module name."); + return false; + } + + return true; +} + function NewProjectDialog::onCreate(%this) { if(%this.validate()) { %title = %this.titleBox.getText(); %directory = %this.dirBox.getText(); + %moduleName = %this.moduleNameBox.getText(); + %author = %this.authorBox.getText(); %description = %this.descBox.getText(); + %core = %this.selectedGameCore(); + // createPath wants a separator on the end -- without one the last folder is + // read as a filename and never made -- and everything else wants none. A + // trailing separator left on %path ends up in the middle of every path + // built from it, and while the engine's own file calls expand that away, + // script side isDirectory does not: it stats the string it is given. %path = makeFullPath(%directory, getMainDotCsDir()); %lastChar = getSubStr(%path, strlen(%path) - 1, 1); - if(%lastChar !$= "\\" && %lastChar !$= "\/") + if(%lastChar $= "\\" || %lastChar $= "/") { - %path = %path @ "\\"; + %path = getSubStr(%path, 0, strlen(%path) - 1); } - createPath(%path); + createPath(%path @ "/"); + + %modulePath = pathConcat(%path, %moduleName); ModuleDatabase.scanModules(pathConcat(getMainDotCsDir(), "library")); ModuleDatabase.CopyModule("AppCore", 1, "AppCore", %path, true); ModuleDatabase.CopyModule("Audio", 1, "Audio", %path, true); - ModuleDatabase.CopyModule("BlankGame", 1, "BlankGame", pathConcat(%path, "BlankGame"), false); + + // Copied under the core's own id, then renamed on the copy. Handing + // CopyModule a different target id would make it rename module.taml to + // .module.taml, which is not the name the rest of the editor + // opens a module definition by, and there is no fileRename in script to + // put it back. Its taml rewriting is also root-only and cannot reach a + // script file at all, which is where the namespace lives. + ModuleDatabase.CopyModule(%core, 1, %core, %modulePath, false); ModuleDatabase.clearDatabase(); // The stock theme and its baked font caches. Not a module: a theme belongs @@ -145,24 +328,22 @@ class = "EditorForm"; // no baked fonts.) pathCopy(pathConcat(getMainDotCsDir(), "library", "themes"), pathConcat(%path, "themes"), false); + ModuleStamper.renameInPlace(%modulePath, %core, %moduleName); + %file = TamlRead(pathConcat(%path, "AppCore", "1", "module.taml")); %file.Project = %title; %file.ProjectDescription = %description; TamlWrite(%file, pathConcat(%path, "AppCore", "1", "module.taml")); + %file.delete(); - %file = TamlRead(pathConcat(%path, "BlankGame", "module.taml")); + %file = TamlRead(pathConcat(%modulePath, "module.taml")); %file.Group = "launch"; %file.Type = "Game Module"; - %file.Author = ""; - TamlWrite(%file, pathConcat(%path, "BlankGame", "module.taml")); - - %data = new ScriptObject() - { - title = %title; - directory = %directory; - description = %description; - icon = pathConcat(%path, "AppCore", %file.Icon); - }; + %file.Author = %author; + %file.Description = %description; + ModuleStamper.clearTemplateMarkers(%file); + TamlWrite(%file, pathConcat(%modulePath, "module.taml")); + %file.delete(); %this.postEvent("ProjectCreated", %directory); %this.onClose(); diff --git a/editor/ProjectManager/scripts/NewModuleDialog.cs b/editor/ProjectManager/scripts/NewModuleDialog.cs index 9133ff9ec..4d7e45cd8 100644 --- a/editor/ProjectManager/scripts/NewModuleDialog.cs +++ b/editor/ProjectManager/scripts/NewModuleDialog.cs @@ -61,17 +61,35 @@ class = "EditorForm"; %allModules = %manager.findModules(false); + // The dropdown shows a display name, and sortByText reorders it, so the id a + // name belongs to is remembered against the name rather than by position. The + // manager and its module definitions are gone by the time a choice is made. for(%i = 0; %i < getWordCount(%allModules); %i++) { %mod = getWord(%allModules, %i); - if(%mod.type $= "template") + if(%mod.Template) { - %this.templateDropDown.addItem(%mod.ModuleID); + %name = ModuleStamper.displayName(%mod); + %this.templateDropDown.addItem(%name); + %this.templateID[%name] = %mod.ModuleID; } } %this.templateDropDown.sortByText(); %this.templateDropDown.insertItem(0, "none"); %this.templateDropDown.setSelected(0); + + %manager.delete(); +} + +function NewModuleDialog::getSelectedTemplate(%this) +{ + %name = %this.templateDropDown.getText(); + if(%name $= "none" || %this.templateID[%name] $= "") + { + return "none"; + } + + return %this.templateID[%name]; } function NewModuleDialog::onDropDownClosed(%this, %dropDown) @@ -93,7 +111,6 @@ class = "EditorForm"; { %this.createButton.active = false; - %module = %this.templateDropDown.getText(); %name = %this.moduleNameBox.getText(); %path = pathConcat(getMainDotCsDir(), ProjectManager.getProjectFolder(), %name); @@ -112,7 +129,7 @@ class = "EditorForm"; { if(%this.validate()) { - %module = %this.templateDropDown.getText(); + %module = %this.getSelectedTemplate(); %name = %this.moduleNameBox.getText(); %path = pathConcat(getMainDotCsDir(), ProjectManager.getProjectFolder(), %name); diff --git a/editor/ProjectManager/scripts/ProjectGamePanel.cs b/editor/ProjectManager/scripts/ProjectGamePanel.cs index 020db772d..f487ecaa2 100644 --- a/editor/ProjectManager/scripts/ProjectGamePanel.cs +++ b/editor/ProjectManager/scripts/ProjectGamePanel.cs @@ -98,10 +98,23 @@ class = "NewModuleDialog"; if(isDirectory(%templatePath)) { pathCopy(%templatePath, %data.path); + + // The id is in the template's script and asset ids too, not just in + // its module.taml, and the engine calls ::. + // Rewriting only the definition leaves a module that loads and then + // does nothing. + ModuleStamper.renameInPlace(%data.path, %data.template, %data.moduleName); + %obj = TamlRead(pathConcat(%data.path, "module.taml")); %obj.ModuleID = %data.moduleName; + + // A copy of a template is a module of its own: it is not something to + // stamp out again, and it is not a Game Core or an Art Pack either. + ModuleStamper.clearTemplateMarkers(%obj); %obj.Type = ""; + TamlWrite(%obj, pathConcat(%data.path, "module.taml")); + %obj.delete(); } } else @@ -117,6 +130,7 @@ class = "NewModuleDialog"; }; createPath(%data.path); TamlWrite(%obj, pathConcat(%data.path, "module.taml")); + %obj.delete(); } ModuleDatabase.scanModules(%data.path); %this.onOpen(ModuleDatabase.findModules(false)); @@ -177,6 +191,12 @@ class = "EditModuleDialog"; { directoryDelete(%modulePath); %modulePath = %newModulePath; + + // The old id is written through the module's own scripts and asset + // ids as well, and the engine calls ::. + // Renaming the folder and the definition alone would leave a module + // that loads and then does nothing. + ModuleStamper.renameInPlace(%modulePath, %moduleID, %data.moduleID); } } echo("Editing Module at " @ %modulePath); @@ -188,6 +208,7 @@ class = "EditModuleDialog"; %file.type = %data.type; %file.author = %data.author; TamlWrite(%file, pathConcat(%modulePath, "module.taml")); + %file.delete(); ModuleDatabase.scanModules(%modulePath, true); %this.card.moduleID = %data.moduleID; %this.card.versionID = %data.versionID; diff --git a/editor/ProjectManager/scripts/ProjectLibraryPanel.cs b/editor/ProjectManager/scripts/ProjectLibraryPanel.cs index 393eab37a..3e9eef41c 100644 --- a/editor/ProjectManager/scripts/ProjectLibraryPanel.cs +++ b/editor/ProjectManager/scripts/ProjectLibraryPanel.cs @@ -24,7 +24,9 @@ function ProjectLibraryPanel::addModule(%this, %module) { - if(%module.type !$= "Template") + // Template modules are stamped out into a project by New Module or New + // Project, not installed alongside it, so they do not belong in this list. + if(!%module.Template) { %this.list.addItemWithID(%this.getModuleName(%module), %module); } diff --git a/library/ArtPack/module.taml b/library/ArtPack/module.taml index 8b6d25ba0..486a0b433 100644 --- a/library/ArtPack/module.taml +++ b/library/ArtPack/module.taml @@ -2,7 +2,9 @@ ModuleId="ArtPack" VersionId="1" BuildID="1" - Type="Template" + Template="1" + Type="Art Pack" + DisplayName="Art Pack" Description="A module to pack your art. How refined." Author="Torque2D"> ::, so a module renamed that way loaded, reported +// its new name everywhere the UI looked, and silently did nothing -- and its +// : ids pointed at a module that no longer existed. Nothing about +// the module.taml those paths wrote looked wrong, which is why this checks the +// script and the gui rather than the definition. +// +// The New Project dialog is the third path and has its own suite; what is +// specific here is that these two go through the Project Manager's own panel, +// against a project folder named by a relative path. +// +// Run: tests/run.ps1 moduleRename ; grep MODREN in tests/logs/. +//----------------------------------------------------------------------------- + +setLogMode(1); +$Scripts::ignoreDSOs = true; +setScriptExecEcho(false); +trace(false); + +function mrCheck(%label, %cond) +{ + if(%cond) echo("MODREN PASS: " @ %label); + else echo("MODREN FAIL: " @ %label); +} + +function mrRead(%path) +{ + %file = new FileObject(); + if(!%file.openForRead(%path)) + { + %file.delete(); + return ""; + } + + %text = ""; + while(!%file.isEOF()) + { + %text = %text @ %file.readLine() @ "\n"; + } + %file.close(); + %file.delete(); + + return %text; +} + +// Case insensitive: Taml writes a dynamic field under the name the string table +// interned, which is lower case. +function mrFileHas(%path, %needle) +{ + return strpos(strlwr(mrRead(%path)), strlwr(%needle)) != -1; +} + +// The three places a module id is written, checked together because a rename +// that gets one and misses another is exactly the failure this is about. +function mrCheckModule(%label, %folder, %id, %gone) +{ + %path = testRoot("moduleRenameSmokeProject/" @ %folder); + + mrCheck(%label @ ": the folder is named for the module", isDirectory(%path)); + mrCheck(%label @ ": module.taml carries the id", mrFileHas(pathConcat(%path, "module.taml"), "ModuleId=\"" @ %id @ "\"")); + mrCheck(%label @ ": create is on the new namespace", mrFileHas(pathConcat(%path, "game.cs"), "function " @ %id @ "::create")); + mrCheck(%label @ ": destroy is on the new namespace", mrFileHas(pathConcat(%path, "game.cs"), "function " @ %id @ "::destroy")); + mrCheck(%label @ ": the asset id in script was rewritten", mrFileHas(pathConcat(%path, "game.cs"), "\"" @ %id @ ":planetfall\"")); + mrCheck(%label @ ": the asset id in the gui was rewritten", mrFileHas(pathConcat(%path, "gui/defaultGui.gui.taml"), %id @ ":torqueBG")); + + mrCheck(%label @ ": module.taml has no trace of " @ %gone, !mrFileHas(pathConcat(%path, "module.taml"), %gone)); + mrCheck(%label @ ": the script has no trace of " @ %gone, !mrFileHas(pathConcat(%path, "game.cs"), %gone)); + mrCheck(%label @ ": the gui has no trace of " @ %gone, !mrFileHas(pathConcat(%path, "gui/defaultGui.gui.taml"), %gone)); +} + +//----------------------------------------------------------------------------- + +testExec("editor/main.cs"); +schedule(2000, 0, "mrStep1"); + +// Spelled out rather than held in a variable: tests/run.ps1 finds the folder to +// delete by reading this file for setProjectFolder("..."), so a name it cannot +// see is a folder it cannot sweep. +function mrStep1() +{ + ProjectManager.setProjectFolder("moduleRenameSmokeProject"); + createPath(testRoot("moduleRenameSmokeProject/")); + + // The BlankGame template depends on Audio, so the last step cannot load the + // renamed module unless Audio is in the project beside it. + ModuleDatabase.scanModules(testRoot("library")); + ModuleDatabase.CopyModule("Audio", 1, "Audio", testRoot("moduleRenameSmokeProject"), true); + ModuleDatabase.clearDatabase(); + + // The dialog offers templates by display name and hands back a module id, so + // the two have to still agree after the ModuleManager it read them from is + // gone. + %width = 500; + %height = 190; + %dialog = new GuiControl() + { + class = "NewModuleDialog"; + superclass = "EditorDialog"; + dialogSize = (%width + 8) SPC (%height + 8); + dialogCanClose = true; + dialogText = "Create Module"; + }; + %dialog.init(%width, %height); + Canvas.pushDialog(%dialog); + + mrCheck("the template dropdown offers Blank Game", %dialog.templateDropDown.findItemText("Blank Game", false) != -1); + mrCheck("the template dropdown offers Art Pack", %dialog.templateDropDown.findItemText("Art Pack", false) != -1); + mrCheck("nothing is offered under its raw module id", %dialog.templateDropDown.findItemText("BlankGame", false) == -1); + mrCheck("no template means no template", %dialog.getSelectedTemplate() $= "none"); + + %dialog.templateDropDown.setSelected(%dialog.templateDropDown.findItemText("Blank Game", false)); + mrCheck("a display name maps back to its module id", %dialog.getSelectedTemplate() $= "BlankGame"); + + %dialog.onClose(); + + // What NewModuleDialog posts when someone picks a template and names it. + %data = new ScriptObject() + { + template = "BlankGame"; + moduleName = "ArcadeCore"; + path = testRoot("moduleRenameSmokeProject/ArcadeCore"); + }; + + ProjectManager.gamePanel.onModuleCreated(%data); + %data.delete(); + + mrCheckModule("new module", "ArcadeCore", "ArcadeCore", "BlankGame"); + + // A copy of a template is a module of its own, not something to stamp out + // again and not a Game Core. + %definition = testRoot("moduleRenameSmokeProject/ArcadeCore/module.taml"); + mrCheck("new module: the template markers are gone", !mrFileHas(%definition, "Template=") && !mrFileHas(%definition, "DisplayName=")); + mrCheck("new module: the game core type is gone", !mrFileHas(%definition, "Type=")); + + schedule(500, 0, "mrStep2"); +} + +// And renaming it again, through Edit Module. +function mrStep2() +{ + %module = ModuleDatabase.findModule("ArcadeCore", 1); + mrCheck("the new module registered", isObject(%module)); + if(!isObject(%module)) + { + echo("MODREN DONE"); + schedule(200, 0, "quit"); + return; + } + + // EditModuleDialog edits the module the card is showing, so that is what has + // to be selected for the panel to act on it. + ProjectManager.gamePanel.card.activeModule = %module; + + %data = new ScriptObject() + { + moduleID = "PinballCore"; + versionID = %module.versionID; + buildID = %module.buildID; + description = "Renamed by the module rename smoke test."; + type = ""; + author = "Smoke Tester"; + }; + + ProjectManager.gamePanel.onModuleEdited(%data); + %data.delete(); + + mrCheck("the old folder is gone", !isDirectory(testRoot("moduleRenameSmokeProject/ArcadeCore"))); + mrCheckModule("renamed module", "PinballCore", "PinballCore", "ArcadeCore"); + + schedule(500, 0, "mrStep3"); +} + +// The proof: load it and see create run. A module whose namespace still said +// ArcadeCore would load without complaint and leave the Canvas empty. +function mrStep3() +{ + ModuleDatabase.scanModules(testRoot("moduleRenameSmokeProject")); + + %module = ModuleDatabase.findModule("PinballCore", 1); + mrCheck("the renamed module registers under its new id", isObject(%module)); + + if(isObject(%module)) + { + ModuleDatabase.loadExplicit("PinballCore", 1); + mrCheck("the module reports itself loaded", ModuleDatabase.isModuleLoaded("PinballCore")); + + %content = Canvas.getContent(); + mrCheck("create ran and put its gui on the Canvas", isObject(%content) && %content.getName() $= "DefaultGui"); + + ModuleDatabase.unloadExplicit("PinballCore", 1); + } + + echo("MODREN DONE"); + schedule(200, 0, "quit"); +} diff --git a/tests/smoke/newProject.cs b/tests/smoke/newProject.cs new file mode 100644 index 000000000..2172b4f46 --- /dev/null +++ b/tests/smoke/newProject.cs @@ -0,0 +1,245 @@ +// New Project smoke test. Drives the New Project dialog and then reads what it +// left on disk. +// +// The thing under test is the rename. A project is stamped out of a library +// game core, and until now the copy kept the template's name -- so every +// project on disk had a module called BlankGame in it. Renaming it is not one +// edit: the id is in module.taml, it is the namespace the engine calls create +// and destroy on in the module's script, and it is the prefix of every asset id +// the module uses, in script and in taml alike. Miss any of those and the +// module still loads, still reports the new name, and silently does nothing. +// +// So the checks below are not "is the id right" but "is the id right in all +// four places", and the last step loads the module for real: if create did not +// fire, the Canvas has no content, and no amount of correct-looking taml makes +// up for that. +// +// Driven by calling the dialog rather than by posting input: what a click would +// be testing is where the boxes ended up, and every one of them is read back by +// name here anyway. +// +// Run: tests/run.ps1 newProject ; grep NEWPROJ in tests/logs/. +//----------------------------------------------------------------------------- + +setLogMode(1); +$Scripts::ignoreDSOs = true; +setScriptExecEcho(false); +trace(false); + +function npCheck(%label, %cond) +{ + if(%cond) echo("NEWPROJ PASS: " @ %label); + else echo("NEWPROJ FAIL: " @ %label); +} + +// FileObject reads through the ResourceManager, which falls back to a real stat +// for a path it has never scanned -- which is every file this test wrote. +function npRead(%path) +{ + %file = new FileObject(); + if(!%file.openForRead(%path)) + { + %file.delete(); + return ""; + } + + %text = ""; + while(!%file.isEOF()) + { + %text = %text @ %file.readLine() @ "\n"; + } + %file.close(); + %file.delete(); + + return %text; +} + +// Case insensitive, and deliberately so. Taml writes a dynamic field under the +// name the string table interned, which is lower case -- AppCore's Project comes +// back out of a round trip as project -- so a case sensitive search would miss +// half of what is being looked for, in both directions. +function npFileHas(%path, %needle) +{ + return strpos(strlwr(npRead(%path)), strlwr(%needle)) != -1; +} + +// Where a control ends, in its parent's coordinates. +function npBottomOf(%control) +{ + return getWord(%control.getPosition(), 1) + getWord(%control.getExtent(), 1); +} + +//----------------------------------------------------------------------------- + +testExec("editor/main.cs"); +schedule(2000, 0, "npStep1"); + +// Spelled out rather than held in a variable: tests/run.ps1 finds the folder to +// delete by reading this file for setProjectFolder("..."), so a name it cannot +// see is a folder it cannot sweep. Everything this test makes is inside it. +function npStep1() +{ + ProjectManager.setProjectFolder("newProjectSmokeProject"); + + // The same size EditorProjectSelector::onNewProject builds it at, because the + // layout checks below are about that size being enough. + %width = 700; + %height = 470; + %dialog = new GuiControl() + { + class = "NewProjectDialog"; + superclass = "EditorDialog"; + dialogSize = (%width + 8) SPC (%height + 8); + dialogCanClose = true; + dialogResizable = false; + dialogText = "New Project"; + }; + %dialog.init(%width, %height); + Canvas.pushDialog(%dialog); + + $npDialog = %dialog; + + // A window spends 34 pixels of its height on the title bar and its border, so + // a control placed from the dialog's own height instead of from what is left + // hangs past the bottom and puts the whole form behind a scroll bar. That is + // invisible in the arithmetic and obvious on screen, which is exactly the kind + // of thing worth pinning down here. + npCheck("the form fits the content pane", npBottomOf(%dialog.form) <= %dialog.contentHeight()); + npCheck("the feedback line fits the content pane", npBottomOf(%dialog.feedback) <= %dialog.contentHeight()); + npCheck("the cancel button fits the content pane", npBottomOf(%dialog.cancelButton) <= %dialog.contentHeight()); + npCheck("the create button fits the content pane", npBottomOf(%dialog.createButton) <= %dialog.contentHeight()); + npCheck("the feedback line clears the buttons", npBottomOf(%dialog.feedback) <= getWord(%dialog.createButton.getPosition(), 1)); + + // Every field says what it is for, on the caption as well as on the input. + npCheck("the title has a tooltip", %dialog.titleBox.tooltip !$= ""); + npCheck("the directory has a tooltip", %dialog.dirBox.tooltip !$= ""); + npCheck("the game core has a tooltip", %dialog.coreDropDown.tooltip !$= ""); + npCheck("the module name has a tooltip", %dialog.moduleNameBox.tooltip !$= ""); + npCheck("the author has a tooltip", %dialog.authorBox.tooltip !$= ""); + npCheck("the description has a tooltip", %dialog.descBox.tooltip !$= ""); + npCheck("the captions carry the tooltip too", %dialog.descBox.getGroup().tooltip $= %dialog.descBox.tooltip); + + // The library ships one game core. Whatever else is in there, Blank Game has + // to be offered, and nothing that is not a game core may be. + npCheck("game core dropdown is populated", %dialog.coreDropDown.getItemCount() > 0); + npCheck("Blank Game is offered by its display name", %dialog.coreDropDown.findItemText("Blank Game", false) != -1); + npCheck("Art Pack is not offered as a game core", %dialog.coreDropDown.findItemText("Art Pack", false) == -1); + npCheck("the selected core is a module id", %dialog.selectedGameCore() $= "BlankGame"); + + // Nothing typed yet, so there is nothing to create. + npCheck("create is inactive while empty", !%dialog.createButton.active); + + schedule(100, 0, "npStep2"); +} + +// The module name follows the title until someone types their own. +function npStep2() +{ + %dialog = $npDialog; + + %dialog.titleBox.setText("New Project Smoke"); + %dialog.onKeyPressed(%dialog.titleBox); + npCheck("module name follows the title", %dialog.moduleNameBox.getText() $= "NewProjectSmokeGame"); + + %dialog.moduleNameBox.setText("SmokeGame"); + %dialog.onKeyPressed(%dialog.moduleNameBox); + %dialog.titleBox.setText("New Project Smoke Test"); + %dialog.onKeyPressed(%dialog.titleBox); + npCheck("an edited module name stops following", %dialog.moduleNameBox.getText() $= "SmokeGame"); + + // Validate stops at the first thing wrong with the form, so the directory + // goes in before the module name is worth asking about. + %dialog.dirBox.setText("newProjectSmokeProject"); + + // A module name becomes a folder, an id, and a script namespace. + %dialog.moduleNameBox.setText("Smoke Game"); + npCheck("a space in the module name is refused", !%dialog.validate()); + %dialog.moduleNameBox.setText("2SmokeGame"); + npCheck("a leading digit is refused", !%dialog.validate()); + %dialog.moduleNameBox.setText("AppCore"); + npCheck("a name already used by the project is refused", !%dialog.validate()); + %dialog.moduleNameBox.setText("SmokeGame"); + + npCheck("no description means no create", !%dialog.validate()); + + %dialog.authorBox.setText("Smoke Tester"); + %dialog.descBox.setText("A project made by the New Project smoke test."); + npCheck("a filled in form validates", %dialog.validate()); + + %dialog.onCreate(); + + schedule(500, 0, "npStep3"); +} + +// What landed on disk. +function npStep3() +{ + %project = testRoot("newProjectSmokeProject"); + %module = pathConcat(%project, "SmokeGame"); + + npCheck("the game module is named after the module name", isDirectory(%module)); + npCheck("no folder is left named after the template", !isDirectory(pathConcat(%project, "BlankGame"))); + npCheck("AppCore came with it", isDirectory(pathConcat(%project, "AppCore"))); + npCheck("Audio came with it", isDirectory(pathConcat(%project, "Audio"))); + npCheck("the stock theme came with it", isDirectory(pathConcat(%project, "themes"))); + + // The module definition kept its own filename. CopyModule renames it to + // .module.taml when the ids differ, which is not the name any of + // the editor's module code opens. + %definition = pathConcat(%module, "module.taml"); + npCheck("the definition is still called module.taml", npFileHas(%definition, "ModuleDefinition")); + npCheck("the id is the module name", npFileHas(%definition, "ModuleId=\"SmokeGame\"")); + npCheck("the definition mentions the template nowhere", !npFileHas(%definition, "BlankGame")); + npCheck("the module launches with the project", npFileHas(%definition, "Group=\"launch\"")); + npCheck("the type says what it is", npFileHas(%definition, "Type=\"Game Module\"")); + npCheck("the author is the one typed in", npFileHas(%definition, "Author=\"Smoke Tester\"")); + npCheck("the description is the one typed in", npFileHas(%definition, "A project made by the New Project smoke test.")); + npCheck("the template's own description is gone", !npFileHas(%definition, "ready for you to craft")); + npCheck("the template markers are gone", !npFileHas(%definition, "Template=") && !npFileHas(%definition, "DisplayName=")); + + // The namespace the engine calls, and an asset id, in a file no taml visitor + // can reach. + %script = pathConcat(%module, "game.cs"); + npCheck("create is on the new namespace", npFileHas(%script, "function SmokeGame::create")); + npCheck("destroy is on the new namespace", npFileHas(%script, "function SmokeGame::destroy")); + npCheck("the asset id in script was rewritten", npFileHas(%script, "\"SmokeGame:planetfall\"")); + npCheck("the script mentions the template nowhere", !npFileHas(%script, "BlankGame")); + + // And an asset id in taml. + %gui = pathConcat(%module, "gui/defaultGui.gui.taml"); + npCheck("the asset id in the gui was rewritten", npFileHas(%gui, "SmokeGame:torqueBG")); + npCheck("the gui mentions the template nowhere", !npFileHas(%gui, "BlankGame")); + + // The project's own identity, which lives on AppCore. + %appCore = pathConcat(%project, "AppCore/1/module.taml"); + npCheck("the project is titled", npFileHas(%appCore, "Project=\"New Project Smoke Test\"")); + npCheck("the project is described", npFileHas(%appCore, "ProjectDescription=\"A project made by the New Project smoke test.\"")); + + schedule(500, 0, "npStep4"); +} + +// The proof the rename is complete: load the module and see create run. A module +// whose namespace still said BlankGame would load without complaint and leave +// the Canvas empty. +function npStep4() +{ + // The whole project, not just the module: loadExplicit pulls the module's + // Audio dependency in, and it has to be registered to be found. + ModuleDatabase.scanModules(testRoot("newProjectSmokeProject")); + %module = ModuleDatabase.findModule("SmokeGame", 1); + npCheck("the renamed module registers under its new id", isObject(%module)); + + if(isObject(%module)) + { + ModuleDatabase.loadExplicit("SmokeGame", 1); + npCheck("the module reports itself loaded", ModuleDatabase.isModuleLoaded("SmokeGame")); + + %content = Canvas.getContent(); + npCheck("create ran and put its gui on the Canvas", isObject(%content) && %content.getName() $= "DefaultGui"); + + ModuleDatabase.unloadExplicit("SmokeGame", 1); + } + + echo("NEWPROJ DONE"); + schedule(200, 0, "quit"); +} From f9af640c9201cbddb65de42fc135e1caf52acfeb Mon Sep 17 00:00:00 2001 From: Greenfire27 Date: Mon, 17 Aug 2026 21:33:50 -0400 Subject: [PATCH 2/3] A declared path spelled the way it was written Make a project and its game module declares Path="Sprites" and Path="Fonts", where the template it was copied from said "sprites" and "fonts". On Windows nobody notices. On Linux those directories do not exist, so an image dropped into sprites/ afterwards is never scanned and never becomes an asset -- silently, with the module.taml sitting right there looking correct. AppCore gets it too, so every project made by this editor has had it. Nothing in the path code was wrong. The fields were TypeString, whose ConsoleSetType interns without caseSens, and StringTable's hash is case insensitive by construction: hashString runs every byte through a to-lower table, so two spellings that differ only in case ALWAYS land in the same bucket, and an insensitive insert then matches the first one there with dStricmp and hands back ITS spelling rather than the one it was asked for. Whether a project came out right depended on whether anything else in the process had already interned "Sprites" -- which is why this looked intermittent and why the template on disk was never wrong. TypeCaseString already existed and already does the right thing -- insert(argv[0], true) -- and is already used on taml serialized classes (GuiControl::text), so this is a field type change rather than new machinery. Declared and Referenced assets take it for Path and Extension; ModuleMergeDefinition for MergePath. ModuleDefinition's ScriptFile and AssetTagsManifest are protected fields, so the setter is the write path and takes the flag directly. The dead C++ setters beside them were changed too, so the classes cannot disagree with themselves later. WHAT IS DELIBERATELY LEFT FOLDED. ModuleId, Group, Type and the create and destroy function names stay TypeString. Those are identifiers rather than paths, and unlike the paths they ARE compared as string table pointers -- ModuleManager does it in nine places for load order, groups, types and dependency resolution. That matching has always been case insensitive, so a project depending on "AppCore=1" resolves against a module whose definition says "appCore" today. Making them case sensitive would break that, and it would present as a missing module rather than as a spelling problem. There is a test asserting ModuleId still folds, so this reads as a decision. Every field changed here was checked for pointer comparison first, because that is the way this fix bites back: StringTableEntry equality is pointer equality, and it only holds because both sides were interned the same way. Once a bucket holds one spelling insensitively and another sensitively, an insensitive insert returns whichever node sits earlier in the chain, so a case sensitive field compared against a value interned the old way can miss. None of these seven are compared -- getPath only ever feeds a dSprintf, getExtension is matched with dStricmp inside the scan, and ScriptFile, AssetTagsManifest and MergePath are only ever tested against EmptyString before being formatted into a path. The ResourceManager and TextureManager dictionaries are the reason the fix stops here. Both HASH by pointer value, so a mismatch does not merely fail a comparison, it never reaches the right bucket at all -- and their keys come from the platform layer's dumpPath, which interns whatever readdir returned. That cluster is self consistent today and has to move as one piece with every platform back end. Worth doing, not worth doing halfway. stringTableCaseTests pins the primitive down in both directions, including the hazard above, so that a later simplification of the flag fails a test that says why it exists. declaredPathCaseTests goes through setDataField rather than the C++ setters, because that is the route TAML takes, and it carries a control: ModuleId is poisoned and set identically and still folds, which is what makes the six passing cases evidence about the field type rather than about the fixture. The New Project suite checks the artifact -- Path="sprites", ScriptFile="game.cs" -- case sensitively, against a project the editor really built. Co-Authored-By: Claude Opus 5 (1M context) --- cmake/EngineSources.cmake | 2 + engine/source/assets/declaredAssets.cc | 13 +- engine/source/assets/declaredAssets.h | 7 +- engine/source/assets/referencedAssets.cc | 8 +- engine/source/assets/referencedAssets.h | 5 +- engine/source/module/moduleDefinition.h | 15 +- engine/source/module/moduleMergeDefinition.cc | 5 +- engine/source/module/moduleMergeDefinition.h | 3 +- .../testing/tests/declaredPathCaseTests.cc | 169 ++++++++++++++++++ .../testing/tests/stringTableCaseTests.cc | 137 ++++++++++++++ tests/smoke/newProject.cs | 16 ++ 11 files changed, 368 insertions(+), 12 deletions(-) create mode 100644 engine/source/testing/tests/declaredPathCaseTests.cc create mode 100644 engine/source/testing/tests/stringTableCaseTests.cc diff --git a/cmake/EngineSources.cmake b/cmake/EngineSources.cmake index e393c032f..d94fccf76 100644 --- a/cmake/EngineSources.cmake +++ b/cmake/EngineSources.cmake @@ -346,6 +346,7 @@ set(TORQUE_ENGINE_SOURCES ${TORQUE_SRC}/testing/tests/animationFrameConversionTests.cc ${TORQUE_SRC}/testing/tests/assetStateCopyTests.cc ${TORQUE_SRC}/testing/tests/bitmapFontParseTests.cc + ${TORQUE_SRC}/testing/tests/declaredPathCaseTests.cc ${TORQUE_SRC}/testing/tests/guiControlReparentTests.cc ${TORQUE_SRC}/testing/tests/guiCursorHotSpotTests.cc ${TORQUE_SRC}/testing/tests/guiFrameStripLayoutTests.cc @@ -362,6 +363,7 @@ set(TORQUE_ENGINE_SOURCES ${TORQUE_SRC}/testing/tests/platformMemoryTests.cc ${TORQUE_SRC}/testing/tests/platformStringTests.cc ${TORQUE_SRC}/testing/tests/simObjectCloneTests.cc + ${TORQUE_SRC}/testing/tests/stringTableCaseTests.cc # ---- platform ---- ${TORQUE_SRC}/platform/CursorManager.cc ${TORQUE_SRC}/platform/Tickable.cc diff --git a/engine/source/assets/declaredAssets.cc b/engine/source/assets/declaredAssets.cc index d888093a6..f1a8b9283 100644 --- a/engine/source/assets/declaredAssets.cc +++ b/engine/source/assets/declaredAssets.cc @@ -39,7 +39,16 @@ void DeclaredAssets::initPersistFields() // Call Parent. Parent::initPersistFields(); - addField("Path", TypeString, Offset(mPath, DeclaredAssets), "" ); - addField("Extension", TypeString, Offset(mExtension, DeclaredAssets), "" ); + // TypeCaseString, not TypeString: both of these are read off a case + // sensitive filesystem and written back to it. TypeString interns without + // caseSens, and the string table's hash is case insensitive, so whichever + // spelling of a name reached the table first is the spelling that comes + // back -- a module declaring "sprites" gets "Sprites" written into its + // module.taml the moment anything else in the process has interned that. + // Safe to make case sensitive here because neither value is ever compared + // as a StringTableEntry: getPath only ever feeds a dSprintf, and + // getExtension is matched with dStricmp inside the scan. + addField("Path", TypeCaseString, Offset(mPath, DeclaredAssets), "" ); + addField("Extension", TypeCaseString, Offset(mExtension, DeclaredAssets), "" ); addField("Recurse", TypeBool, Offset(mRecurse, DeclaredAssets), "" ); } diff --git a/engine/source/assets/declaredAssets.h b/engine/source/assets/declaredAssets.h index 1f3baf85a..4cc57ca35 100644 --- a/engine/source/assets/declaredAssets.h +++ b/engine/source/assets/declaredAssets.h @@ -50,9 +50,12 @@ class DeclaredAssets : public SimObject static void initPersistFields(); - inline void setPath( const char* pPath ) { mPath = StringTable->insert( pPath ); } + // Case sensitive, to match the TypeCaseString fields these back. See + // initPersistFields for why a filesystem name cannot be interned the + // ordinary way. + inline void setPath( const char* pPath ) { mPath = StringTable->insert( pPath, true ); } inline StringTableEntry getPath( void ) const { return mPath; } - inline void setExtension( const char* pPath ) { mExtension = StringTable->insert( pPath ); } + inline void setExtension( const char* pPath ) { mExtension = StringTable->insert( pPath, true ); } inline StringTableEntry getExtension( void ) const { return mExtension; } inline void setRecurse( const bool recurse ) { mRecurse = recurse; } inline bool getRecurse( void ) const { return mRecurse; } diff --git a/engine/source/assets/referencedAssets.cc b/engine/source/assets/referencedAssets.cc index f5807de2b..958ea8fe9 100644 --- a/engine/source/assets/referencedAssets.cc +++ b/engine/source/assets/referencedAssets.cc @@ -39,7 +39,11 @@ void ReferencedAssets::initPersistFields() // Call Parent. Parent::initPersistFields(); - addField("Path", TypeString, Offset(mPath, ReferencedAssets), "" ); - addField("Extension", TypeString, Offset(mExtension, ReferencedAssets), "" ); + // TypeCaseString for the same reason DeclaredAssets uses it: these name a + // directory and an extension on disk, and TypeString would hand back + // whichever spelling of them the string table saw first. Neither is ever + // compared as a StringTableEntry. + addField("Path", TypeCaseString, Offset(mPath, ReferencedAssets), "" ); + addField("Extension", TypeCaseString, Offset(mExtension, ReferencedAssets), "" ); addField("Recurse", TypeBool, Offset(mRecurse, ReferencedAssets), "" ); } \ No newline at end of file diff --git a/engine/source/assets/referencedAssets.h b/engine/source/assets/referencedAssets.h index 09a12dba1..1e345290a 100644 --- a/engine/source/assets/referencedAssets.h +++ b/engine/source/assets/referencedAssets.h @@ -50,9 +50,10 @@ class ReferencedAssets : public SimObject static void initPersistFields(); - inline void setPath( const char* pPath ) { mPath = StringTable->insert( pPath ); } + // Case sensitive, to match the TypeCaseString fields these back. + inline void setPath( const char* pPath ) { mPath = StringTable->insert( pPath, true ); } inline StringTableEntry getPath( void ) const { return mPath; } - inline void setExtension( const char* pPath ) { mExtension = StringTable->insert( pPath ); } + inline void setExtension( const char* pPath ) { mExtension = StringTable->insert( pPath, true ); } inline StringTableEntry getExtension( void ) const { return mExtension; } inline void setRecurse( const bool recurse ) { mRecurse = recurse; } inline bool getRecurse( void ) const { return mRecurse; } diff --git a/engine/source/module/moduleDefinition.h b/engine/source/module/moduleDefinition.h index 62380bcac..23bdfbad0 100755 --- a/engine/source/module/moduleDefinition.h +++ b/engine/source/module/moduleDefinition.h @@ -151,7 +151,16 @@ class ModuleDefinition : public SimSet inline StringTableEntry getModuleType( void ) const { return mModuleType; } inline void setDependencies( const typeModuleDependencyVector& dependencies ) { if ( checkUnlocked() ) { mDependencies.clear(); mDependencies.merge(dependencies); } } inline const typeModuleDependencyVector& getDependencies( void ) const { return mDependencies; } - inline void setScriptFile( const char* pScriptFile ) { if ( checkUnlocked() ) { mScriptFile = StringTable->insert(pScriptFile); } } + // Case sensitive: this is a filename on disk, and the string table's hash + // is case insensitive, so an ordinary insert returns whichever spelling of + // the name reached the table first -- which on Linux is a script the module + // then cannot find. Safe here because mScriptFile is only ever compared + // against EmptyString (moduleManager.cc) and otherwise formatted into a + // path. NOT done for ModuleId, Group, Type or the create/destroy function + // names below: those are identifiers, they ARE compared as string table + // pointers, and dependency resolution has always matched them regardless of + // case. + inline void setScriptFile( const char* pScriptFile ) { if ( checkUnlocked() ) { mScriptFile = StringTable->insert(pScriptFile, true); } } inline StringTableEntry getScriptFile( void ) const { return mScriptFile; } inline void setCreateFunction( const char* pCreateFunction ) { if ( checkUnlocked() ) { mCreateFunction = StringTable->insert(pCreateFunction); } } inline StringTableEntry getCreateFunction( void ) const { return mCreateFunction; } @@ -160,7 +169,9 @@ class ModuleDefinition : public SimSet inline SimObjectId getScopeSet( void ) const { return mScopeSet; } /// Module assets. - inline void setAssetTagsManifest( const char* pTagsAssetManifest ) { if ( checkUnlocked() ) { mAssetTagsManifest = StringTable->insert(pTagsAssetManifest); } } + // Case sensitive for the same reason as setScriptFile: a filename. Only + // ever handed to Con::expandPath, never compared. + inline void setAssetTagsManifest( const char* pTagsAssetManifest ) { if ( checkUnlocked() ) { mAssetTagsManifest = StringTable->insert(pTagsAssetManifest, true); } } inline StringTableEntry getAssetTagsManifest( void ) const { return mAssetTagsManifest; } inline typeModuleAssetsVector& getModuleAssets( void ) { return mModuleAssets; } diff --git a/engine/source/module/moduleMergeDefinition.cc b/engine/source/module/moduleMergeDefinition.cc index 218ed7809..901515197 100755 --- a/engine/source/module/moduleMergeDefinition.cc +++ b/engine/source/module/moduleMergeDefinition.cc @@ -45,5 +45,8 @@ void ModuleMergeDefinition::initPersistFields() Parent::initPersistFields(); /// Module merge. - addField( "MergePath", TypeString, Offset(mModuleMergePath, ModuleMergeDefinition), "The path where the modules to be merged can be found." ); + // TypeCaseString: a directory on disk, so it has to come back out of the + // string table spelled the way it went in. Never compared as a string table + // pointer -- moduleManager.cc only reads it to build a path from. + addField( "MergePath", TypeCaseString, Offset(mModuleMergePath, ModuleMergeDefinition), "The path where the modules to be merged can be found." ); } diff --git a/engine/source/module/moduleMergeDefinition.h b/engine/source/module/moduleMergeDefinition.h index 58509b86f..5edee274c 100755 --- a/engine/source/module/moduleMergeDefinition.h +++ b/engine/source/module/moduleMergeDefinition.h @@ -45,7 +45,8 @@ class ModuleMergeDefinition : public SimObject static void initPersistFields(); /// Module merge. - inline void setModuleMergePath( const char* pModuleMergePath ) { mModuleMergePath = StringTable->insert(pModuleMergePath); } + // Case sensitive, to match the TypeCaseString field this backs. + inline void setModuleMergePath( const char* pModuleMergePath ) { mModuleMergePath = StringTable->insert(pModuleMergePath, true); } inline StringTableEntry getModuleMergePath( void ) const { return mModuleMergePath; } /// Declare Console Object. diff --git a/engine/source/testing/tests/declaredPathCaseTests.cc b/engine/source/testing/tests/declaredPathCaseTests.cc new file mode 100644 index 000000000..df5d68741 --- /dev/null +++ b/engine/source/testing/tests/declaredPathCaseTests.cc @@ -0,0 +1,169 @@ +//----------------------------------------------------------------------------- +// Copyright (c) 2013 GarageGames, LLC +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to +// deal in the Software without restriction, including without limitation the +// rights to use, copy, modify, merge, publish, distribute, sublicense, and/or +// sell copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS +// IN THE SOFTWARE. +//----------------------------------------------------------------------------- + +// We don't want tests in a shipping version. +#ifndef TORQUE_SHIPPING + +#ifndef _UNIT_TESTING_H_ +#include "testing/unitTesting.h" +#endif + +#ifndef _DECLARED_ASSETS_H_ +#include "assets/declaredAssets.h" +#endif + +#ifndef _REFERENCED_ASSETS_H_ +#include "assets/referencedAssets.h" +#endif + +#ifndef _MODULE_DEFINITION_H_ +#include "module/moduleDefinition.h" +#endif + +//----------------------------------------------------------------------------- +// A declared path comes back out spelled the way it went in. +// +// The bug this pins down was found by making a project: BlankGame declares +// Path="sprites" and Path="fonts", and the copy written into a new project said +// Path="Sprites" and Path="Fonts". On Windows nobody notices. On Linux those +// directories do not exist, so an asset a person later puts in sprites/ is +// silently never scanned. +// +// Nothing in the path code was wrong. The field was TypeString, which interns +// without caseSens, and the string table's hash is case insensitive by +// construction -- so the first spelling of a name to reach the table becomes +// the spelling everyone gets. Whether a project came out right depended on what +// else the editor had loaded first, which is why it looked intermittent. +// +// These go through setDataField rather than the C++ setters on purpose. That is +// the same route TAML takes, so it exercises the console type that was actually +// changed rather than the setter beside it. +// +// Every spelling here is unique to its own test: the string table is a +// process-wide singleton with no way to remove an entry, so a shared name would +// make these depend on each other's order. +//----------------------------------------------------------------------------- + +// Put a spelling in the table so the field has something to be folded into. In +// the wild this is another module, an editor script, or a directory scan -- +// anything at all, which is the point. +static void poisonStringTable( const char* pSpelling ) +{ + StringTable->insert( pSpelling ); +} + +TEST( DeclaredPathCaseTests, ADeclaredPathKeepsItsSpelling ) +{ + poisonStringTable( "DpcSpritesOne" ); + + DeclaredAssets declared; + declared.setDataField( StringTable->insert( "Path" ), NULL, "dpcspritesone" ); + + ASSERT_STREQ( declared.getPath(), "dpcspritesone" ); + + SUCCEED(); +} + +TEST( DeclaredPathCaseTests, ADeclaredExtensionKeepsItsSpelling ) +{ + poisonStringTable( "DpcAssetTamlTwo" ); + + DeclaredAssets declared; + declared.setDataField( StringTable->insert( "Extension" ), NULL, "dpcassettamltwo" ); + + ASSERT_STREQ( declared.getExtension(), "dpcassettamltwo" ); + + SUCCEED(); +} + +TEST( DeclaredPathCaseTests, AReferencedPathKeepsItsSpelling ) +{ + poisonStringTable( "DpcSpritesThree" ); + + ReferencedAssets referenced; + referenced.setDataField( StringTable->insert( "Path" ), NULL, "dpcspritesthree" ); + + ASSERT_STREQ( referenced.getPath(), "dpcspritesthree" ); + + SUCCEED(); +} + +TEST( DeclaredPathCaseTests, AReferencedExtensionKeepsItsSpelling ) +{ + poisonStringTable( "DpcAssetTamlFour" ); + + ReferencedAssets referenced; + referenced.setDataField( StringTable->insert( "Extension" ), NULL, "dpcassettamlfour" ); + + ASSERT_STREQ( referenced.getExtension(), "dpcassettamlfour" ); + + SUCCEED(); +} + +TEST( DeclaredPathCaseTests, AModuleScriptFileKeepsItsSpelling ) +{ + poisonStringTable( "DpcGameFive.cs" ); + + ModuleDefinition definition; + definition.setDataField( StringTable->insert( "ScriptFile" ), NULL, "dpcgamefive.cs" ); + + ASSERT_STREQ( definition.getScriptFile(), "dpcgamefive.cs" ); + + SUCCEED(); +} + +TEST( DeclaredPathCaseTests, AModuleAssetTagsManifestKeepsItsSpelling ) +{ + poisonStringTable( "DpcTagsSix.taml" ); + + ModuleDefinition definition; + definition.setDataField( StringTable->insert( "AssetTagsManifest" ), NULL, "dpctagssix.taml" ); + + ASSERT_STREQ( definition.getAssetTagsManifest(), "dpctagssix.taml" ); + + SUCCEED(); +} + +// THE BOUNDARY, asserted so it is a decision rather than an oversight. +// +// A module Id is an identifier, not a path. It IS compared as a string table +// pointer -- ModuleManager does it in nine places for load order, groups, types +// and dependency resolution -- and those comparisons have always been case +// insensitive, so a project depending on "AppCore=1" resolves against a module +// spelling itself "appCore". Making this case sensitive to match the paths above +// would quietly break that, and the breakage would look like a missing module +// rather than a spelling problem. +// +// So ModuleId stays TypeString, and this test is here to say that on purpose. +TEST( DeclaredPathCaseTests, AModuleIdIsStillFoldedBecauseItIsAnIdentifier ) +{ + poisonStringTable( "DpcModuleSeven" ); + + ModuleDefinition definition; + definition.setDataField( StringTable->insert( "ModuleId" ), NULL, "dpcmoduleseven" ); + + ASSERT_STREQ( definition.getModuleId(), "DpcModuleSeven" ); + + SUCCEED(); +} + +#endif // TORQUE_SHIPPING diff --git a/engine/source/testing/tests/stringTableCaseTests.cc b/engine/source/testing/tests/stringTableCaseTests.cc new file mode 100644 index 000000000..633dedc20 --- /dev/null +++ b/engine/source/testing/tests/stringTableCaseTests.cc @@ -0,0 +1,137 @@ +//----------------------------------------------------------------------------- +// Copyright (c) 2013 GarageGames, LLC +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to +// deal in the Software without restriction, including without limitation the +// rights to use, copy, modify, merge, publish, distribute, sublicense, and/or +// sell copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS +// IN THE SOFTWARE. +//----------------------------------------------------------------------------- + +// We don't want tests in a shipping version. +#ifndef TORQUE_SHIPPING + +#ifndef _UNIT_TESTING_H_ +#include "testing/unitTesting.h" +#endif + +#ifndef _STRINGTABLE_H_ +#include "string/stringTable.h" +#endif + +//----------------------------------------------------------------------------- +// What the string table does to the case of a string it is given. +// +// insert() defaults to caseSens = false, and the hash is case insensitive by +// construction -- hashString runs every byte through a to-lower table -- so two +// spellings that differ only in case always land in the same bucket. An +// insensitive insert then matches the first one there with dStricmp and hands +// back ITS spelling, not the one it was asked for. +// +// For a name that is only ever compared, that is the point of the table. For a +// FILESYSTEM PATH it is data corruption: whichever subsystem happened to intern +// "Sprites" first decides that a module declaring "sprites" gets "Sprites" +// written back to its module.taml, and on a case sensitive filesystem that +// directory then does not exist. +// +// These pin down both halves -- the trap and the flag that avoids it -- because +// the fix is a one word argument at each call site, and an argument that +// silently stops mattering is worth a test. +// +// Every string here is prefixed and unique to its own test. The table is a +// process-wide singleton with no way to remove an entry, so a shared spelling +// would make these tests depend on each other's order. +//----------------------------------------------------------------------------- + +TEST( StringTableCaseTests, AnInsensitiveInsertReturnsTheSpellingAlreadyInTheTable ) +{ + StringTableEntry first = StringTable->insert( "StCaseTrapSprites" ); + StringTableEntry second = StringTable->insert( "stcasetrapsprites" ); + + // One entry, not two. + ASSERT_EQ( first, second ); + + // And the caller that asked for lower case did not get lower case back. + ASSERT_STREQ( second, "StCaseTrapSprites" ); + + SUCCEED(); +} + +TEST( StringTableCaseTests, ASensitiveInsertKeepsTheSpellingItWasGiven ) +{ + StringTableEntry mixed = StringTable->insert( "StCaseKeepSprites", true ); + StringTableEntry lower = StringTable->insert( "stcasekeepsprites", true ); + + ASSERT_NE( mixed, lower ); + ASSERT_STREQ( mixed, "StCaseKeepSprites" ); + ASSERT_STREQ( lower, "stcasekeepsprites" ); + + SUCCEED(); +} + +// The order that does the damage in the wild: something else gets there first. +TEST( StringTableCaseTests, ASensitiveInsertSurvivesAnotherSpellingArrivingFirst ) +{ + StringTable->insert( "StCasePoisonFonts" ); + + StringTableEntry kept = StringTable->insert( "stcasepoisonfonts", true ); + + ASSERT_STREQ( kept, "stcasepoisonfonts" ); + + SUCCEED(); +} + +// Both spellings stay reachable afterwards, which is what makes it safe to store +// one of each: a case sensitive lookup finds the one it names. +TEST( StringTableCaseTests, BothSpellingsRemainDistinctUnderLookup ) +{ + StringTable->insert( "StCaseLookupParticles", true ); + StringTable->insert( "stcaselookupparticles", true ); + + ASSERT_STREQ( StringTable->lookup( "StCaseLookupParticles", true ), "StCaseLookupParticles" ); + ASSERT_STREQ( StringTable->lookup( "stcaselookupparticles", true ), "stcaselookupparticles" ); + + SUCCEED(); +} + +// THE HAZARD THE FIX INTRODUCES, pinned down so it is not discovered later. +// +// StringTableEntry equality is pointer equality, and that only holds because +// both sides were interned the same way. Once one spelling is in the table +// insensitively and the other sensitively, an insensitive insert of EITHER +// spelling returns whichever node sits earlier in the bucket -- so a case +// sensitive field compared by pointer against a value interned the old way can +// miss. +// +// This is why the flag belongs only on values that are paths, and why every +// pointer comparison against such a field has to be checked when one is changed. +TEST( StringTableCaseTests, AnInsensitiveInsertCanMissACaseSensitiveEntry ) +{ + // The old-style intern, first into the bucket. + StringTableEntry existing = StringTable->insert( "StCaseHazardGui" ); + + // The new-style one, a second node in the same bucket. + StringTableEntry sensitive = StringTable->insert( "stcasehazardgui", true ); + ASSERT_NE( existing, sensitive ); + + // A caller still interning the old way gets the FIRST node, whichever + // spelling it asks for -- so comparing it against the sensitive entry fails. + ASSERT_EQ( StringTable->insert( "stcasehazardgui" ), existing ); + ASSERT_NE( StringTable->insert( "stcasehazardgui" ), sensitive ); + + SUCCEED(); +} + +#endif // TORQUE_SHIPPING diff --git a/tests/smoke/newProject.cs b/tests/smoke/newProject.cs index 2172b4f46..1435e0fd6 100644 --- a/tests/smoke/newProject.cs +++ b/tests/smoke/newProject.cs @@ -63,6 +63,12 @@ function npFileHas(%path, %needle) return strpos(strlwr(npRead(%path)), strlwr(%needle)) != -1; } +// Case sensitive, for the one thing here where case IS the subject. +function npFileHasExact(%path, %needle) +{ + return strpos(npRead(%path), %needle) != -1; +} + // Where a control ends, in its parent's coordinates. function npBottomOf(%control) { @@ -197,6 +203,16 @@ function npStep3() npCheck("the template's own description is gone", !npFileHas(%definition, "ready for you to craft")); npCheck("the template markers are gone", !npFileHas(%definition, "Template=") && !npFileHas(%definition, "DisplayName=")); + // The declared paths are directories on a case sensitive filesystem, and a + // taml round trip used to fold them to whatever spelling the string table + // happened to hold -- so a project came out declaring Sprites and Fonts, + // and anything put in sprites/ afterwards was never scanned. Checked + // exactly, because the whole point is the spelling. + npCheck("the declared sprites path keeps its spelling", npFileHasExact(%definition, "Path=\"sprites\"")); + npCheck("the declared fonts path keeps its spelling", npFileHasExact(%definition, "Path=\"fonts\"")); + npCheck("the declared extensions keep their spelling", npFileHasExact(%definition, "Extension=\"image.taml\"")); + npCheck("the script file keeps its spelling", npFileHasExact(%definition, "ScriptFile=\"game.cs\"")); + // The namespace the engine calls, and an asset id, in a file no taml visitor // can reach. %script = pathConcat(%module, "game.cs"); From cd879a1129f8831ca2dd0ae83e7e95d26a849c7f Mon Sep 17 00:00:00 2001 From: Greenfire27 Date: Mon, 17 Aug 2026 23:03:31 -0400 Subject: [PATCH 3/3] The folder a scan names is the folder you can open BlankGame declares eight asset paths and shipped three of them, so every project made from it warned four times the first time it was opened -- about sprites, fonts and particles, directories the template never had. Adding them is the whole of the content change here, and each one carries a readme saying what belongs in it and how a file there becomes an asset, because a template is something a person opens and reads. Adding them turned up the rest of this commit. A directory called fonts came back out of readdir as "Fonts". Not sometimes: always, on this platform, since the string table's hash is case insensitive by construction and an ordinary insert returns whichever spelling of a name reached the table first. Two of them get there during STATIC INITIALISATION, before any of the engine runs -- SpriteBatch interns "Sprites" as a taml node name and guiProfileTheme interns "Fonts" as a field group -- so sprites and fonts, the two most ordinary names an asset folder can have, were the two that could never survive a scan. That is why a copied module.taml said Path="Sprites", and why directoryDelete could not recurse into a folder it had just been told about: it was handed a name nothing could stat. So the platform layer interns what readdir gives it case sensitively, and so do ResManager::getPaths, the zip and openFileForWrite paths, and the ResDictionary's own path normalisation. THOSE HAD TO MOVE TOGETHER. The dictionary derives its bucket from the POINTER VALUES of path and name and compares them by pointer, so a half-applied change does not merely compare false -- it hashes into the wrong bucket and reports a file that exists as missing. The four in resourceDictionary are the path key, the rest are the file key, and either one alone would have been worse than the bug. The four editor themes are part of this rather than incidental to it. Each asked for "^EditorCore/Themes//Fonts" while the directory on disk is fonts, and that is where $GUI::fontCacheDirectory comes from. It worked only because the fold rescued it -- guiProfiles.cs asks for the same LabCoat directory in lower case two files away, which is the codebase disagreeing with itself. Left alone, they would have become a real missing-font-cache bug the moment lookups stopped folding, so a latent bug and its disguise are removed in one go. WHAT IS STILL DELIBERATELY FOLDED is unchanged from the previous commit: ModuleId, Group, Type and the create and destroy function names are identifiers, they are compared as string table pointers in nine places in ModuleManager, and dependency resolution has always matched them regardless of case. directoryScanCaseTests writes a real sprites/readme.md and fonts/readme.md, puts the capitalised spellings in the table by hand so the collision cannot be absent by luck, and then asks for the one thing all of this is about: that a caller can open what the scan named. Its last two lines delete the tree the scan just enumerated, which is the exact step that failed in the wild. Only Linux is changed. The same intern sites exist in all six platform back ends and are wrong in the same way, but Windows and macOS are case insensitive at the filesystem, so it costs them nothing today and I cannot run their tests. Co-Authored-By: Claude Opus 5 (1M context) --- cmake/EngineSources.cmake | 1 + .../EditorCore/Themes/BaseTheme/BaseTheme.cs | 2 +- .../Themes/ForestRobe/ForestRobeTheme.cs | 2 +- .../EditorCore/Themes/LabCoat/LabCoatTheme.cs | 2 +- .../Themes/TorqueSuit/TorqueSuitTheme.cs | 2 +- .../source/io/resource/resourceDictionary.cc | 22 ++- engine/source/io/resource/resourceManager.cc | 12 +- .../source/platformX86UNIX/x86UNIXFileio.cc | 27 ++- .../testing/tests/directoryScanCaseTests.cc | 174 ++++++++++++++++++ library/BlankGame/fonts/readme.md | 25 +++ library/BlankGame/particles/readme.md | 28 +++ library/BlankGame/sprites/readme.md | 37 ++++ tests/smoke/newProject.cs | 8 + 13 files changed, 323 insertions(+), 19 deletions(-) create mode 100644 engine/source/testing/tests/directoryScanCaseTests.cc create mode 100644 library/BlankGame/fonts/readme.md create mode 100644 library/BlankGame/particles/readme.md create mode 100644 library/BlankGame/sprites/readme.md diff --git a/cmake/EngineSources.cmake b/cmake/EngineSources.cmake index d94fccf76..d011fe1f9 100644 --- a/cmake/EngineSources.cmake +++ b/cmake/EngineSources.cmake @@ -347,6 +347,7 @@ set(TORQUE_ENGINE_SOURCES ${TORQUE_SRC}/testing/tests/assetStateCopyTests.cc ${TORQUE_SRC}/testing/tests/bitmapFontParseTests.cc ${TORQUE_SRC}/testing/tests/declaredPathCaseTests.cc + ${TORQUE_SRC}/testing/tests/directoryScanCaseTests.cc ${TORQUE_SRC}/testing/tests/guiControlReparentTests.cc ${TORQUE_SRC}/testing/tests/guiCursorHotSpotTests.cc ${TORQUE_SRC}/testing/tests/guiFrameStripLayoutTests.cc diff --git a/editor/EditorCore/Themes/BaseTheme/BaseTheme.cs b/editor/EditorCore/Themes/BaseTheme/BaseTheme.cs index dc382c672..fdccc061f 100644 --- a/editor/EditorCore/Themes/BaseTheme/BaseTheme.cs +++ b/editor/EditorCore/Themes/BaseTheme/BaseTheme.cs @@ -81,7 +81,7 @@ %this.font[1] = "raleway";//Most common font %this.font[2] = "black ops one";//Title fontType %this.font[3] = "fira code semibold";//Code and console font - %this.fontDirectory = expandPath("^EditorCore/Themes/BaseTheme/Fonts"); + %this.fontDirectory = expandPath("^EditorCore/Themes/BaseTheme/fonts"); %this.fontSize = 20; %this.color1 = "10 10 10 255";//Most commonly used for backgrounds diff --git a/editor/EditorCore/Themes/ForestRobe/ForestRobeTheme.cs b/editor/EditorCore/Themes/ForestRobe/ForestRobeTheme.cs index 8a0cd9b1b..71a5d43e2 100644 --- a/editor/EditorCore/Themes/ForestRobe/ForestRobeTheme.cs +++ b/editor/EditorCore/Themes/ForestRobe/ForestRobeTheme.cs @@ -6,7 +6,7 @@ %this.font[1] = "raleway";//Most common font %this.font[2] = "cinzel decorative bold";//Title fontType %this.font[3] = "fira code semibold";//Code and console font - %this.fontDirectory = expandPath("^EditorCore/Themes/ForestRobe/Fonts"); + %this.fontDirectory = expandPath("^EditorCore/Themes/ForestRobe/fonts"); %this.fontSize = 20; %this.color1 = "43 53 66 255"; diff --git a/editor/EditorCore/Themes/LabCoat/LabCoatTheme.cs b/editor/EditorCore/Themes/LabCoat/LabCoatTheme.cs index 38675b7ce..3657f43de 100644 --- a/editor/EditorCore/Themes/LabCoat/LabCoatTheme.cs +++ b/editor/EditorCore/Themes/LabCoat/LabCoatTheme.cs @@ -6,7 +6,7 @@ %this.font[1] = "roboto";//Most common font %this.font[2] = "zen dots";//Title fontType %this.font[3] = "share tech mono";//Code and console font - %this.fontDirectory = expandPath("^EditorCore/Themes/LabCoat/Fonts"); + %this.fontDirectory = expandPath("^EditorCore/Themes/LabCoat/fonts"); %this.fontSize = 20; %this.color1 = "255 255 255 255"; diff --git a/editor/EditorCore/Themes/TorqueSuit/TorqueSuitTheme.cs b/editor/EditorCore/Themes/TorqueSuit/TorqueSuitTheme.cs index 24e29c3e6..730a4435e 100644 --- a/editor/EditorCore/Themes/TorqueSuit/TorqueSuitTheme.cs +++ b/editor/EditorCore/Themes/TorqueSuit/TorqueSuitTheme.cs @@ -6,7 +6,7 @@ %this.font[1] = "raleway";//Most common font %this.font[2] = "Audiowide";//Title fontType %this.font[3] = "vt323";//Code and console font - %this.fontDirectory = expandPath("^EditorCore/Themes/TorqueSuit/Fonts"); + %this.fontDirectory = expandPath("^EditorCore/Themes/TorqueSuit/fonts"); %this.fontSize = 22; %this.color1 = "34 19 30 255"; diff --git a/engine/source/io/resource/resourceDictionary.cc b/engine/source/io/resource/resourceDictionary.cc index 55b0724e3..604f87c13 100755 --- a/engine/source/io/resource/resourceDictionary.cc +++ b/engine/source/io/resource/resourceDictionary.cc @@ -45,6 +45,20 @@ ResDictionary::~ResDictionary() delete[] hashTable; } +// NOTE the bucket is derived from the POINTER VALUES of path and file, and the +// comparison in find() below is pointer equality too. That only works while +// every string table entry reaching this dictionary was interned the same way, +// which is why the four insert() calls in this file, ResManager::getPaths, the +// zip and openFileForWrite paths in resourceManager.cc, and the platform layer's +// dumpPath all pass caseSens = true together. Flip one of them alone and a +// lookup does not merely compare false -- it hashes into the wrong bucket and +// the file is reported missing. +// +// They are case sensitive because these name real files: the string table's hash +// is case insensitive by construction, so an ordinary insert returns whichever +// spelling of a name reached the table first, and a directory called fonts came +// back as "Fonts" because guiProfileTheme.cc interns that word as a field group +// during static initialisation. S32 ResDictionary::hash(StringTableEntry path, StringTableEntry file) { return ((U32)((((dsize_t)path) >> 2) + (((dsize_t)file) >> 2) )) % hashTableSize; @@ -56,7 +70,7 @@ void ResDictionary::insert(ResourceObject *obj, StringTableEntry path, StringTab { char fullPath[1024]; Platform::makeFullPathName(path, fullPath, sizeof(fullPath)); - path = StringTable->insert(fullPath); + path = StringTable->insert(fullPath, true); } obj->name = file; @@ -102,7 +116,7 @@ ResourceObject* ResDictionary::find(StringTableEntry path, StringTableEntry name { char fullPath[1024]; Platform::makeFullPathName(path, fullPath, sizeof(fullPath)); - path = StringTable->insert(fullPath); + path = StringTable->insert(fullPath, true); } for(ResourceObject *walk = hashTable[hash(path, name)]; walk; walk = walk->nextEntry) @@ -117,7 +131,7 @@ ResourceObject* ResDictionary::find(StringTableEntry path, StringTableEntry name { char fullPath[1024]; Platform::makeFullPathName(path, fullPath, sizeof(fullPath)); - path = StringTable->insert(fullPath); + path = StringTable->insert(fullPath, true); } for(ResourceObject *walk = hashTable[hash(path, name)]; walk; walk = walk->nextEntry) @@ -132,7 +146,7 @@ ResourceObject* ResDictionary::find(StringTableEntry path, StringTableEntry name { char fullPath[1024]; Platform::makeFullPathName(path, fullPath, sizeof(fullPath)); - path = StringTable->insert(fullPath); + path = StringTable->insert(fullPath, true); } for(ResourceObject *walk = hashTable[hash(path, name)]; walk; walk = walk->nextEntry) diff --git a/engine/source/io/resource/resourceManager.cc b/engine/source/io/resource/resourceManager.cc index 5db8c0ac7..5dde0836b 100755 --- a/engine/source/io/resource/resourceManager.cc +++ b/engine/source/io/resource/resourceManager.cc @@ -310,15 +310,15 @@ static void getPaths (const char *fullPath, StringTableEntry & path, if (!ptr) { path = NULL; - fileName = StringTable->insert (fullPath); + fileName = StringTable->insert (fullPath, true); } else { S32 len = (S32)(ptr - fullPath); dStrncpy (buf, fullPath, len); buf[len] = 0; - fileName = StringTable->insert (ptr + 1); - path = StringTable->insert (buf); + fileName = StringTable->insert (ptr + 1, true); + path = StringTable->insert (buf, true); } } @@ -397,8 +397,8 @@ bool ResManager::scanZip (ResourceObject * zipObject) continue; pPathEnd[0] = '\0'; - const char * path = StringTable->insert(zipPath); - const char * file = StringTable->insert(pPathEnd + 1); + const char * path = StringTable->insert(zipPath, true); + const char * file = StringTable->insert(pPathEnd + 1, true); ResourceObject *ro = createZipResource(path, file, zipObject->zipPath, zipObject->zipName); @@ -1297,7 +1297,7 @@ bool ResManager::openFileForWrite (FileStream & stream, const char *fileName, U3 return false; // create a resource for the file. - ResourceObject *ro = createResource (StringTable->insert (path), StringTable->insert (file)); + ResourceObject *ro = createResource (StringTable->insert (path, true), StringTable->insert (file, true)); ro->flags = ResourceObject::File; ro->fileOffset = 0; ro->fileSize = 0; diff --git a/engine/source/platformX86UNIX/x86UNIXFileio.cc b/engine/source/platformX86UNIX/x86UNIXFileio.cc index 6fc48d41f..560747d4d 100755 --- a/engine/source/platformX86UNIX/x86UNIXFileio.cc +++ b/engine/source/platformX86UNIX/x86UNIXFileio.cc @@ -380,10 +380,10 @@ Platform::FileInfo& rInfo = fileVector.last(); if (relativePath) - rInfo.pFullPath = StringTable->insert(relativePath); + rInfo.pFullPath = StringTable->insert(relativePath, true); else - rInfo.pFullPath = StringTable->insert(path); - rInfo.pFileName = StringTable->insert(fEntry->d_name); + rInfo.pFullPath = StringTable->insert(path, true); + rInfo.pFileName = StringTable->insert(fEntry->d_name, true); rInfo.fileSize = fStat.st_size; //dPrintf("Adding file: %s/%s\n", rInfo.pFullPath, rInfo.pFileName); } @@ -1061,6 +1061,23 @@ return false; // Add path to our return list (provided it is valid). + // + // Interned CASE SENSITIVELY, and that is load bearing on this platform. The + // string table's hash is case insensitive by construction, so an ordinary + // insert returns whichever spelling of a name reached the table first -- and + // several spellings get there during static initialisation, before any of + // this runs: SpriteBatch.cc interns "Sprites" as a taml node name and + // guiProfileTheme.cc interns "Fonts" as a field group. The result was that a + // directory genuinely called sprites or fonts came back out of readdir as + // "Sprites" or "Fonts", and every caller that then tried to open it on a case + // sensitive filesystem failed: deleteDirectory could not recurse into it, + // getDirectoryList reported a name nothing could stat, and scanModules + // skipped it. + // + // The file names in RecurseDumpPath above are interned the same way, and the + // two have to stay in step with ResManager::getPaths and the ResDictionary: + // that dictionary hashes by pointer value, so a half-applied change misses + // the bucket rather than merely comparing false. if (!Platform::isExcludedDirectory(subPath)){ if (noBasePath){ // No base path requested: store only non-empty subpaths, and NEVER the @@ -1069,10 +1086,10 @@ // code fell into the else below for the empty-subPath root call and // pushed the base path, so getDirectoryList() returned just the path. if (subPath && (dStrncmp(subPath, "", 1) != 0)) - directoryVector.push_back(StringTable->insert(subPath)); + directoryVector.push_back(StringTable->insert(subPath, true)); } else { // There is a base path. Store the concatenated path. - directoryVector.push_back(StringTable->insert(Path)); + directoryVector.push_back(StringTable->insert(Path, true)); } } diff --git a/engine/source/testing/tests/directoryScanCaseTests.cc b/engine/source/testing/tests/directoryScanCaseTests.cc new file mode 100644 index 000000000..2b59f0542 --- /dev/null +++ b/engine/source/testing/tests/directoryScanCaseTests.cc @@ -0,0 +1,174 @@ +//----------------------------------------------------------------------------- +// Copyright (c) 2013 GarageGames, LLC +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to +// deal in the Software without restriction, including without limitation the +// rights to use, copy, modify, merge, publish, distribute, sublicense, and/or +// sell copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS +// IN THE SOFTWARE. +//----------------------------------------------------------------------------- + +// We don't want tests in a shipping version. +#ifndef TORQUE_SHIPPING + +#ifndef _UNIT_TESTING_H_ +#include "testing/unitTesting.h" +#endif + +#ifndef _PLATFORM_H_ +#include "platform/platform.h" +#endif + +#ifndef _PLATFORM_FILEIO_H_ +#include "platform/platformFileIO.h" +#endif + +#ifndef _STRINGTABLE_H_ +#include "string/stringTable.h" +#endif + +// platform.h only forward declares Vector, and both scans return one. +#ifndef _VECTOR_H_ +#include "collection/vector.h" +#endif + +//----------------------------------------------------------------------------- +// A directory scan reports the names that are actually on disk. +// +// This is the half of the string table case problem that cannot be fixed by +// choosing a better field type, because the strings do not come from a taml +// file -- they come from readdir. The string table's hash is case insensitive +// by construction, so an ordinary insert of a name read off the filesystem +// returns whichever spelling of it reached the table first, and several arrive +// during static initialisation before any of this runs: SpriteBatch interns +// "Sprites" as a taml node name and guiProfileTheme interns "Fonts" as a field +// group. The result on a case sensitive filesystem was that a directory +// genuinely called sprites or fonts was enumerated as "Sprites" or "Fonts", and +// every caller that then tried to open it failed -- deleteDirectory could not +// recurse into it, and getDirectoryList named something nothing could stat. +// +// The names below are deliberately the real ones. A test using invented +// spellings would pass whether or not the words that actually collide are +// handled, and those words are the whole point. +//----------------------------------------------------------------------------- + +#define SCANCASE_ROOT "_unitTestScanCase_RemoveMe" + +static void scanCasePath( char* buffer, U32 bufferSize, const char* relative ) +{ + if ( relative == NULL ) + { + dSprintf( buffer, bufferSize, "%s/%s", + Platform::getCurrentDirectory(), SCANCASE_ROOT ); + return; + } + + dSprintf( buffer, bufferSize, "%s/%s/%s", + Platform::getCurrentDirectory(), SCANCASE_ROOT, relative ); +} + +static bool scanCaseWriteFile( const char* path ) +{ + Platform::createPath( path ); + + File file; + if ( file.open( path, File::Write ) != File::Ok ) + return false; + + U32 written = 0; + const bool ok = file.write( 2, "hi", &written ) == File::Ok; + file.close(); + return ok; +} + +// Was any of the names the scan returned spelled this way? +static bool scanFound( Vector& names, const char* spelling ) +{ + for ( S32 i = 0; i < names.size(); i++ ) + { + if ( dStrcmp( names[i], spelling ) == 0 ) + return true; + } + + return false; +} + +static bool scanFoundFile( Vector& files, const char* spelling ) +{ + for ( S32 i = 0; i < files.size(); i++ ) + { + if ( dStrcmp( files[i].pFileName, spelling ) == 0 ) + return true; + } + + return false; +} + +TEST( DirectoryScanCaseTests, AScanReportsTheSpellingOnDisk ) +{ + char root[1024]; + scanCasePath( root, sizeof( root ), NULL ); + + if ( Platform::isDirectory( root ) ) + Platform::deleteDirectory( root ); + + // Lower case on disk, and every one of these words is already in the string + // table capitalised by the time any of this runs. + char file[1024]; + scanCasePath( file, sizeof( file ), "sprites/readme.md" ); + ASSERT_TRUE( scanCaseWriteFile( file ) ) << "Could not write the scratch file."; + scanCasePath( file, sizeof( file ), "fonts/readme.md" ); + ASSERT_TRUE( scanCaseWriteFile( file ) ) << "Could not write the scratch file."; + + // Belt and braces: make sure the capitalised spellings really are in the + // table, so this test cannot pass by the collision simply not existing. + StringTable->insert( "Sprites" ); + StringTable->insert( "Fonts" ); + StringTable->insert( "README.md" ); + + // With a trailing separator, exactly as the getDirectoryList binding calls + // it. Without one the back-end returns its children as "/sprites" rather + // than "sprites", which is a separate quirk and not what this is about. + char rootSlash[1024]; + dSprintf( rootSlash, sizeof( rootSlash ), "%s/", root ); + + Vector directories; + ASSERT_TRUE( Platform::dumpDirectories( rootSlash, directories, 0, true ) ) << "Could not scan the scratch root."; + + ASSERT_TRUE( scanFound( directories, "sprites" ) ) << "sprites came back spelled some other way."; + ASSERT_TRUE( scanFound( directories, "fonts" ) ) << "fonts came back spelled some other way."; + ASSERT_FALSE( scanFound( directories, "Sprites" ) ) << "A directory was reported under a spelling that is not on disk."; + ASSERT_FALSE( scanFound( directories, "Fonts" ) ) << "A directory was reported under a spelling that is not on disk."; + + // And the file names inside them. + char fontsDir[1024]; + scanCasePath( fontsDir, sizeof( fontsDir ), "fonts" ); + + Vector files; + ASSERT_TRUE( Platform::dumpPath( fontsDir, files, 0 ) ) << "Could not scan the scratch directory."; + + ASSERT_TRUE( scanFoundFile( files, "readme.md" ) ) << "readme.md came back spelled some other way."; + ASSERT_FALSE( scanFoundFile( files, "README.md" ) ) << "A file was reported under a spelling that is not on disk."; + + // What all of the above is really about: a caller can open what the scan + // named. This is the step that failed in the wild -- deleteDirectory + // recursing into a child it had just been told about. + ASSERT_TRUE( Platform::deleteDirectory( root ) ) << "Could not delete a tree the scan had just enumerated."; + ASSERT_FALSE( Platform::isDirectory( root ) ) << "The tree is still there."; + + SUCCEED(); +} + +#endif // TORQUE_SHIPPING diff --git a/library/BlankGame/fonts/readme.md b/library/BlankGame/fonts/readme.md new file mode 100644 index 000000000..25029cdbc --- /dev/null +++ b/library/BlankGame/fonts/readme.md @@ -0,0 +1,25 @@ +# fonts + +Bitmap fonts your game draws text with live here. + +A font is a `.fnt` description plus the page images it refers to, and it becomes +an asset when a `.font.taml` names it: + +```xml + +``` + +A control then asks for it as `YourModule:titleFont` — where `YourModule` is the +`ModuleId` at the top of the `module.taml` next to this folder. + +This folder is scanned **recursively**, so subfolders are fine. + +The `.fnt` files themselves come from a bitmap font tool; BMFont's text format is +what the engine reads. The editor's font tools are under **Ctrl + ~**, and the +theme a project ships with keeps its own baked font caches separately, in +`themes/`, so nothing here is needed just to get text on screen. + +This file is only here to keep the folder around in an empty project. Delete it +whenever you like. diff --git a/library/BlankGame/particles/readme.md b/library/BlankGame/particles/readme.md new file mode 100644 index 000000000..50767855a --- /dev/null +++ b/library/BlankGame/particles/readme.md @@ -0,0 +1,28 @@ +# particles + +Particle effects — explosions, smoke, sparks, rain — live here. + +Each effect is a single `.particle.taml` describing one or more emitters and how +every one of their properties changes over the life of the effect: + +```xml + + ... + +``` + +A `ParticlePlayer` in your scene then plays it by asset id, +`YourModule:playerDeath` — where `YourModule` is the `ModuleId` at the top of the +`module.taml` next to this folder. + +This folder is scanned **recursively**, so subfolders are fine. + +These are not files to write by hand. Open the editor with **Ctrl + ~**: the +Asset Manager's particle editor plays the effect live while you drag its curves, +which is the only practical way to tune one. + +This file is only here to keep the folder around in an empty project. Delete it +whenever you like. diff --git a/library/BlankGame/sprites/readme.md b/library/BlankGame/sprites/readme.md new file mode 100644 index 000000000..55c8abf5e --- /dev/null +++ b/library/BlankGame/sprites/readme.md @@ -0,0 +1,37 @@ +# sprites + +Your game's images and the animations built from them live here. + +An image file is not an asset on its own. It becomes one when a `.image.taml` +sits beside it naming it: + +```xml + +``` + +An animation is a `.animation.taml` that names frames out of one of those +images: + +```xml + +``` + +Anywhere your game asks for an image or an animation, it names it +`YourModule:playerShip` — where `YourModule` is the `ModuleId` at the top of the +`module.taml` next to this folder. + +This folder is scanned **recursively**, so arrange it into subfolders however +suits you; the asset ids do not change when a file moves. + +You do not have to write any of this by hand. Open the editor with **Ctrl + ~** +and the Asset Manager will create the files, cut a sprite sheet into frames, and +preview an animation as you build it. + +This file is only here to keep the folder around in an empty project. Delete it +whenever you like. diff --git a/tests/smoke/newProject.cs b/tests/smoke/newProject.cs index 1435e0fd6..d6fa09630 100644 --- a/tests/smoke/newProject.cs +++ b/tests/smoke/newProject.cs @@ -208,6 +208,14 @@ function npStep3() // happened to hold -- so a project came out declaring Sprites and Fonts, // and anything put in sprites/ afterwards was never scanned. Checked // exactly, because the whole point is the spelling. + // Every path the definition declares has to exist in the copy, or the asset + // manager warns about each missing one the first time the project is opened + // -- four warnings on a brand new project, which is a poor first thing to + // see and hides a real one when it turns up. + npCheck("the declared sprites folder came with it", isDirectory(pathConcat(%module, "sprites"))); + npCheck("the declared fonts folder came with it", isDirectory(pathConcat(%module, "fonts"))); + npCheck("the declared particles folder came with it", isDirectory(pathConcat(%module, "particles"))); + npCheck("the declared sprites path keeps its spelling", npFileHasExact(%definition, "Path=\"sprites\"")); npCheck("the declared fonts path keeps its spelling", npFileHasExact(%definition, "Path=\"fonts\"")); npCheck("the declared extensions keep their spelling", npFileHasExact(%definition, "Extension=\"image.taml\""));