← Back to team overview

kicad-developers team mailing list archive

PATCH: 3D resolver

 

The attached patch does the following:

1. Ensures ${KISYS3DMOD} is in the resolver's path list even if
it is not defined in KiCad's internal path list (Configure Paths).
This fixes a bug which was described on the dev list but not
reported to the bug tracker.

2. ENV VARs paths defined by the shell take precedence over
what is defined within KiCad; this is consistent with typical
UNIX use: ENV_VAR=something pcbnew test.kicad_pcb

3. If a user defines a path via ${ENV_VAR}/som/file.wrl where
ENV_VAR is an arbitrary environment variable (as opposed to
KISYS3DMOD and others defined via KiCad's Configure Paths)
then that ENV_VAR is automatically added to the list of
base search paths. Previous behavior was to only include
KISYS3DMOD and ENV_VARS defined via Configure Paths.

4. General code cleanup removing functions which were not
necessary since wxFileName already performed the required
tasks.

5. Fix a bug in the file modification time check where a path to
a non-existent file fires a wxASSERT within wxWidgets.

- Cirilo
=== modified file '3d-viewer/3d_cache/3d_cache.cpp'
--- 3d-viewer/3d_cache/3d_cache.cpp	2016-04-17 22:35:32 +0000
+++ 3d-viewer/3d_cache/3d_cache.cpp	2016-04-19 23:32:32 +0000
@@ -225,19 +225,23 @@
     if( mi != m_CacheMap.end() )
     {
         wxFileName fname( full3Dpath );
-        wxDateTime fmdate = fname.GetModificationTime();
         bool reload = false;
 
-        if( fmdate != mi->second->modTime )
+        if( fname.FileExists() )
         {
-            unsigned char hashSum[20];
-            getSHA1( full3Dpath, hashSum );
-            mi->second->modTime = fmdate;
+            wxDateTime fmdate = fname.GetModificationTime();
 
-            if( !isSHA1Same( hashSum, mi->second->sha1sum ) )
+            if( fmdate != mi->second->modTime )
             {
-                mi->second->SetSHA1( hashSum );
-                reload = true;
+                unsigned char hashSum[20];
+                getSHA1( full3Dpath, hashSum );
+                mi->second->modTime = fmdate;
+
+                if( !isSHA1Same( hashSum, mi->second->sha1sum ) )
+                {
+                    mi->second->SetSHA1( hashSum );
+                    reload = true;
+                }
             }
         }
 

=== modified file '3d-viewer/3d_cache/3d_filename_resolver.cpp'
--- 3d-viewer/3d_cache/3d_filename_resolver.cpp	2016-04-17 22:35:32 +0000
+++ 3d-viewer/3d_cache/3d_filename_resolver.cpp	2016-04-20 09:06:02 +0000
@@ -171,12 +171,13 @@
     // the user may change this later with a call to SetProjectDir()
 
     S3D_ALIAS lpath;
-    lpath.m_alias = _( "${KIPRJMOD}" );
-    lpath.m_pathvar = _( "${KIPRJMOD}" );
+    lpath.m_alias = "${KIPRJMOD}";
+    lpath.m_pathvar = "${KIPRJMOD}";
     lpath.m_pathexp = m_curProjDir;
     m_Paths.push_back( lpath );
     wxFileName fndummy;
     wxUniChar psep = fndummy.GetPathSeparator();
+    bool hasKISYS3DMOD = false;
 
     // iterate over the list of internally defined ENV VARs
     // and add existing paths to the resolver
@@ -202,7 +203,15 @@
                 continue;
             }
 
-            fndummy.Assign( mS->second.GetValue(), "" );
+            wxString pathVal;
+
+            // ensure system ENV VARs supercede internally defined vars
+            if( wxGetEnv( mS->first, &pathVal ) && wxDirExists( pathVal ) )
+                fndummy.Assign( pathVal, "" );
+            else
+                fndummy.Assign( mS->second.GetValue(), "" );
+
+            fndummy.Normalize();
 
             if( !fndummy.DirExists() )
             {
@@ -214,9 +223,12 @@
             tmp.Append( mS->first );
             tmp.Append( "}" );
 
+            if( tmp == "${KISYS3DMOD}" )
+                hasKISYS3DMOD = true;
+
             lpath.m_alias =  tmp;
             lpath.m_pathvar = tmp;
-            lpath.m_pathexp = mS->second.GetValue();
+            lpath.m_pathexp = fndummy.GetFullPath();
 
             if( !lpath.m_pathexp.empty() && psep == *lpath.m_pathexp.rbegin() )
                 lpath.m_pathexp.erase( --lpath.m_pathexp.end() );
@@ -227,6 +239,30 @@
         }
     }
 
+    // special case: if KISYSMOD is not internally defined but is defined by
+    // the system, then create an entry here
+    wxString envar;
+
+    if( !hasKISYS3DMOD && wxGetEnv( "KISYS3DMOD", &envar ) )
+    {
+        lpath.m_alias = "${KISYS3DMOD}";
+        lpath.m_pathvar = "${KISYS3DMOD}";
+        fndummy.Assign( envar, "" );
+        fndummy.Normalize();
+
+        if( fndummy.DirExists() )
+        {
+            lpath.m_pathexp = fndummy.GetFullPath();
+
+            if( !lpath.m_pathexp.empty() && psep == *lpath.m_pathexp.rbegin() )
+                lpath.m_pathexp.erase( --lpath.m_pathexp.end() );
+
+            if( !lpath.m_pathexp.empty() )
+                m_Paths.push_back( lpath );
+        }
+
+    }
+
     if( !m_ConfigDir.empty() )
         readPathList();
 
@@ -268,6 +304,7 @@
 wxString S3D_FILENAME_RESOLVER::ResolvePath( const wxString& aFileName )
 {
     wxCriticalSectionLocker lock( lock3D_resolver );
+
     if( aFileName.empty() )
         return wxEmptyString;
 
@@ -289,43 +326,40 @@
     tname.Replace( wxT( "/" ), wxT( "\\" ) );
     #endif
 
-    // this case covers full paths and paths relative to
-    // the current working directory (which is not necessarily
+    wxFileName tmpFN( tname );
+
+    // in the case of absolute filenames we don't store a map item
+    if( !aFileName.StartsWith( wxT( "${" ) ) && tmpFN.IsAbsolute() )
+    {
+        if( tmpFN.FileExists() )
+            return tname;
+
+        return wxEmptyString;
+    }
+
+    tmpFN.Normalize();
+
+    // this case covers full paths, leading expanded vars, and paths
+    // relative to the current working directory (which is not necessarily
     // the current project directory)
-    if( wxFileName::FileExists( tname ) )
+    if( tmpFN.FileExists() )
     {
-        wxFileName tmp( tname );
-
-        if( tmp.Normalize() )
-            tname = tmp.GetFullPath();
-
+        tmpFN.Normalize();
+        tname = tmpFN.GetFullPath();
         m_NameMap.insert( std::pair< wxString, wxString > ( aFileName, tname ) );
 
+        // special case: if a path begins with ${ENV_VAR} but is not in the
+        // resolver's path list then add it
+        if( aFileName.StartsWith( "${" ) )
+            checkEnvVarPath( aFileName );
+
         return tname;
     }
 
-    // at this point aFileName:
-    // a. is a legacy ${} shortened name
-    // b. an aliased shortened name
-    // c. cannot be determined
-
+    // if a path begins with ${ENV_VAR} and is not resolved then the file
+    // either does not exist or the ENV_VAR is not defined
     if( aFileName.StartsWith( wxT( "${" ) ) )
     {
-        wxFileName tmp( aFileName );
-
-        if( tmp.Normalize() )
-        {
-            tname = tmp.GetFullPath();
-            m_NameMap.insert( std::pair< wxString, wxString > ( aFileName, tname ) );
-
-            return tname;
-        }
-        else if( resolveVirtualEnv( aFileName, tname ) )
-        {
-            m_NameMap.insert( std::pair< wxString, wxString > ( aFileName, tname ) );
-            return tname;
-        }
-
         if( !( m_errflags & ERRFLG_ENVPATH ) )
         {
             m_errflags |= ERRFLG_ENVPATH;
@@ -338,6 +372,10 @@
         return wxEmptyString;
     }
 
+    // at this point aFileName is:
+    // a. an aliased shortened name or
+    // b. cannot be determined
+
     std::list< S3D_ALIAS >::const_iterator sPL = m_Paths.begin();
     std::list< S3D_ALIAS >::const_iterator ePL = m_Paths.end();
 
@@ -345,49 +383,47 @@
     // note: this is not necessarily the same as the current working
     // directory, which has already been checked. This case accounts
     // for partial paths which do not contain ${KIPRJMOD}.
-    if( !sPL->m_pathexp.empty() )
+    // This check is performed before checking the path relative to
+    // ${KISYS3DMOD} so that users can potentially override a model
+    // within ${KISYS3DMOD}
+    if( !sPL->m_pathexp.empty() && !tname.StartsWith( ":" ) )
     {
-        wxFileName fpath( wxFileName::DirName( sPL->m_pathexp ) );
-        wxString fullPath = fpath.GetPathWithSep() + tname;
+        tmpFN.Assign( sPL->m_pathexp, "" );
+        wxString fullPath = tmpFN.GetPathWithSep() + tname;
 
         if( wxFileName::FileExists( fullPath ) )
         {
-            wxFileName tmp( fullPath );
-
-            if( tmp.Normalize() )
-                tname = tmp.GetFullPath();
-
+            tmpFN.Assign( fullPath );
+            tmpFN.Normalize();
+            tname = tmpFN.GetFullPath();
             m_NameMap.insert( std::pair< wxString, wxString > ( aFileName, tname ) );
 
             return tname;
         }
+
     }
 
-    // ${ENV_VAR} paths have already been checked; skip all but
-    // ${KISYS3DMOD}, since legacy behavior was to check if paths
-    // were relative to ${KISYS3DMOD}
+    // check the partial path relative to ${KISYS3DMOD} (legacy behavior)
+    if( !tname.StartsWith( ":" ) )
+    {
+        wxFileName fpath;
+        wxString fullPath( "${KISYS3DMOD}" );
+        fullPath.Append( fpath.GetPathSeparator() );
+        fullPath.Append( tname );
+        fpath.Assign( fullPath );
+
+        if( fpath.Normalize() && fpath.FileExists() )
+        {
+            tname = fpath.GetFullPath();
+            m_NameMap.insert( std::pair< wxString, wxString > ( aFileName, tname ) );
+            return tname;
+        }
+
+    } while( 0 );
+
+    // ${ENV_VAR} paths have already been checked; skip them
     while( sPL != ePL && sPL->m_alias.StartsWith( "${" ) )
-    {
-        if( sPL->m_alias == "${KISYS3DMOD}" )
-        {
-            wxFileName fpath( wxFileName::DirName( sPL->m_pathexp ) );
-            wxString fullPath = fpath.GetPathWithSep() + tname;
-
-            if( wxFileName::FileExists( fullPath ) )
-            {
-                wxFileName tmp( fullPath );
-
-                if( tmp.Normalize() )
-                    tname = tmp.GetFullPath();
-
-                m_NameMap.insert( std::pair< wxString, wxString > ( aFileName, tname ) );
-
-                return tname;
-            }
-        }
-
         ++sPL;
-    }
 
     // at this point the filename must contain an alias or else it is invalid
     wxString alias;         // the alias portion of the short filename
@@ -424,7 +460,6 @@
                     tname = tmp.GetFullPath();
 
                 m_NameMap.insert( std::pair< wxString, wxString > ( aFileName, tname ) );
-
                 return tname;
             }
         }
@@ -721,47 +756,54 @@
 }
 
 
-bool S3D_FILENAME_RESOLVER::resolveVirtualEnv( const wxString& aFileName, wxString& aFullPath )
+void S3D_FILENAME_RESOLVER::checkEnvVarPath( const wxString& aPath )
 {
-    aFullPath.clear();
-
-    if( !aFileName.StartsWith( "${" ) )
-        return false;
-
-    size_t eDelim = aFileName.find( '}' );
-
-    if( eDelim == wxString::npos || eDelim + 2 >= aFileName.length() )
-        return false;
-
-    wxString tPath = aFileName.substr( 0, eDelim + 1 );
-
+    if( !aPath.StartsWith( "${" ) )
+        return;
+
+    size_t pEnd = aPath.find( "}" );
+
+    if( pEnd == wxString::npos )
+        return;
+
+    wxString envar = aPath.substr( 0, pEnd + 1 );
+
+    // check if the alias exists; if not then add it to the end of the
+    // env var section of the path list
     std::list< S3D_ALIAS >::const_iterator sPL = m_Paths.begin();
     std::list< S3D_ALIAS >::const_iterator ePL = m_Paths.end();
 
     while( sPL != ePL )
     {
+        if( sPL->m_alias == envar )
+            return;
+
         if( !sPL->m_alias.StartsWith( "${" ) )
-            return false;
-
-        if( sPL->m_alias == tPath )
-        {
-            tPath.Append( aFileName.substr( eDelim + 2 ) );
-            wxFileName tFile( tPath );
-            tFile.Normalize();
-
-            if( tFile.FileExists() )
-            {
-                aFullPath = tFile.GetFullPath();
-                return true;
-            }
-
-            return false;
-        }
+            break;
 
         ++sPL;
     }
 
-    return false;
+    S3D_ALIAS lpath;
+    lpath.m_alias = envar;
+    lpath.m_pathvar = lpath.m_alias;
+    wxFileName tmpFN( lpath.m_alias, "" );
+    wxUniChar psep = tmpFN.GetPathSeparator();
+    tmpFN.Normalize();
+
+    if( !tmpFN.DirExists() )
+        return;
+
+    lpath.m_pathexp = tmpFN.GetFullPath();
+
+    if( !lpath.m_pathexp.empty() && psep == *lpath.m_pathexp.rbegin() )
+        lpath.m_pathexp.erase( --lpath.m_pathexp.end() );
+
+    if( lpath.m_pathexp.empty() )
+        return;
+
+    m_Paths.insert( sPL, lpath );
+    return;
 }
 
 

=== modified file '3d-viewer/3d_cache/3d_filename_resolver.h'
--- 3d-viewer/3d_cache/3d_filename_resolver.h	2016-04-17 22:35:32 +0000
+++ 3d-viewer/3d_cache/3d_filename_resolver.h	2016-04-20 07:14:22 +0000
@@ -99,12 +99,12 @@
     bool writePathList( void );
 
     /**
-     * Function resolveVirtualEnv
-     * extracts the ${ENV_VAR} component of aFileName and puts a
-     * resolved path in aFullPath if the ${ENV_VAR} exists in the
-     * alias list and the referenced file exists.
+     * Function checkEnvVarPath
+     * checks the ${ENV_VAR} component of a path and adds
+     * it to the resolver's path list if it is not yet in
+     * the list
      */
-    bool resolveVirtualEnv( const wxString& aFileName, wxString& aFullPath );
+    void checkEnvVarPath( const wxString& aPath );
 
 public:
     S3D_FILENAME_RESOLVER();


Follow ups