Hello Gerrit,
On 01/10/2011 10:25 AM, Gerrit Voß wrote:
> On Fri, 2011-01-07 at 17:19 -0600, Carsten Neumann wrote:
>> I've a patch set (attached) that adds support for reading containers
>> (not just nodes) to the SceneFileHandler and all SceneFileTypes in trunk
>> (there was some talk about this a while back).
>
> + std::ifstream is(filename.c_str(), std::ios_base::in |
> + std::ios_base::binary);
> +
> + OSG::OSGSceneFileType::ContainerVecType fcVec =
> + OSG::OSGSceneFileType::the().readContainer(
> + SceneFileHandler::the(), is, filename, resolver);
> +
> + if(fcVec.empty() == false)
> + returnValue = fcVec.front();
>
> ok, I'm not that happy with pushing this kind of stuff to the rest
> of the code (replicating it), e.g. why did the Char * signature have
> to go ?
I assume you don't really care that much about the Char* part, but about
the SFT not taking a file name any more? Hm, I can add that back in and
have the SFT base class implement it in terms of the stream interface, ok?
> Similar can't we keep the return one container variant in the
> SFH/SFT for convenience. For both reading and writing, it is a nuisance
> to deal with the vector wrapper in case one is only interested in one
> container.
ok, I'll add a single container version. Again the SFT base class can
implement it in terms of the multi container version, that way it is
enough to implement the multi container read/write functions for a
loader and get the others for free.
>> I've taken the opportunity to also do some cleanup on the
>> SFHandler/SFType interface, the most notable change is that the handler
>> now gets passed down to the SFType. The motivation here is that
>> currently it is not possible to safely load more than one file in
>> parallel [1], because there is only one SFH and loaders access the
>> singleton whenever they need to pull in additional files (changing e.g.
>> the state of the PathHandler).
>> If we allow the SFH to be cloned, each clone can operate in parallel
>> with the others, as long as the loaders only operate on the instance
>> passed to them. Of course for convenience a global instance remains
>> accessible with SceneFileHandler::the().
>
> hmm, in general I'm ok with the changes, but I'm not sure if it
> gets us all the way for all loaders, as quite some store stuff
> in the SFT (killing any par loading).
Don't think so (after the patch that is). It also makes
read()/readContainer() const for SFT, so they either use function local
variables or instantiate some loader object to store stuff in.
> And passing the SceneFileHandler
> around as a parameter gets kind of painful for complex loaders so one
> would be tempted to store it somewhere. Which, for complex loaders, is
> not that much of a problem as they instantiate something on the fly
> anyway. But for a more general solution for all loaders it seems the
> better way around it would be to get rid of the 'static' scene file
> types (only leave a small registration part) and let them be
> instantiated by the SFH, including passing the cloned SFH, during
> loading.
ok, so the SFT would register something like a create() function with
the SFH, which gets passed a few parameters including the calling SFH
instance? Sounds good.
>> Some things that I'd also like to add (so not in the patch yet):
>>
>> - For the parallel load to also work for images, I want to give the SFH
>> a pointer to an associated ImageFileHandler, that then gets used by the
>> loaders instead of going through the singleton.
>
> should be ok.
>
> A general question, as you currently pass the SFH down as 'this',
> later on this has to involve some form of locking so the cloning does
> not happen while something else modifies the global SFH from which you
> clone ?
>
> Another one, as IIRC it was mentioned/half discussed some time ago that
> the general search path handling is kind of scattered, would it be a
> good time (read do we have the time) to revised (or at least remember)
> it in this context ?
not sure what you mean with scattered? It seems to be mostly contained
in SFH::initPathHandler(). Can you refresh my memory what the problem was?
>> - Do we want to keep the read/write callback in SFH? What are they good
>> for anyway, it looks as if they hijack the loading process?
>
> A better way would maybe be to provide a generic callback SFT which
> can easily be registered for the corresponding suffixes instead of
> pushing this to the SFH ?
well, the funny thing is that the callbacks get passed the SFT as an
argument... - Is anybody using this? What's is/was the purpose?
>> - Do we want to keep the progress callbacks?
>
> hmm, are they actually working with useful numbers for all loaders ?
sort of, I guess. It does check the stream position and compares with
the end position to determine progress, so it does not necessarily need
much support from the loader implementations IIUC.
>> - If we keep any of the above, can I convert them to use boost::function?
>
> that should never be a problem, it's just something we did not come
> around doing.
ok.
>> - I would like to have some support for caching in the loader
>> infrastructure [2]. My current thinking is to define an abstract
>> ContainerCache:
>>
>> class ContainerCache
>> {
>> typedef std::vector<FieldContainerUnrecPtr> ContainerVecType;
>>
>> virtual Int32 insert(const ContainerVecType&containers,
>> const std::string&fileName,
>> const std::string&name = "",
>> const FieldContainerType&type =
>> FieldContainer::getClassType()) = 0;
>>
>> virtual Int32 lookup(const std::string&fileName,
>> const std::string&name = "",
>> const FieldContainerType&type =
>> FieldContainer::getClassType()) const = 0;
>>
>> virtual ContainerVecType clone(Int32 handle) const = 0;
>>
>>
>> // plus some bookkeeping stuff like clear(), empty()
>> };
>>
>> and leave it up to applications to provide an implementation that meets
>> their requirements. The name and type argument are meant to be more or
>> less optional, just to give the cache implementation a better hint what
>> is being asked for. SFH could then first query the cache if it has
>> something suitable and then obtain a clone (shallow/deep up to the cache
>> implementation/app) to use.
>
> have to think about this a little ;), so far it seems ok.
ok, I'll keep posting updates to the patch set as it grows.
Cheers,
Carsten
------------------------------------------------------------------------------
Gaining the trust of online customers is vital for the success of any company
that requires sensitive data to be transmitted over the Web. Learn how to
best implement a security strategy that keeps consumers' information secure
and instills the confidence they need to proceed with transactions.
http://p.sf.net/sfu/oracle-sfdevnl
_______________________________________________
Opensg-core mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensg-core
